On Sat, Dec 6, 2025 at 2:27 PM Mark Michelson via dev <
[email protected]> wrote:
> The lflow_table_add_lflow() function takes 13 arguments. Many helper
> macros exist to facilitate filling in those 13 arguments without having
> to spell them all out every time. Unfortunately, the amount of different
> macros has gotten a bit out of hand. Some macros, like
> ovn_lflow_add_with_hint__() have misleading names. Some macros, like
> ovn_lflow_add_drop_with_lport_hint_and_desc() have horribly unwieldy
> names. Trying to add a new macro to the list is a minefield since you
> need to cherry-pick exactly which arguments the new macro should require
> and which ones the macro should fill in automatically.
>
> This series seeks to simplify things by getting rid of all helper macros
> except for ovn_lflow_add() and ovn_lflow_add_default_drop(). We
> accomplish this by changing ovn_lflow_add() into a variadic macro.
> All invocations of ovn_lflow_add() require a minimum of 7 arguments:
> * lflow table
> * ovn_datapath
> * stage
> * priority
> * match
> * actions
> * lflow_ref
>
> Additional fields can be specified by adding any number of additional
> arguments using a series of helper macros to add specific field types.
> The idea behind this is that any argument beyond the 7th can be any of
> the available fields. This gives the flexibility to add lflows in any
> number of combinations without having to define hyper-specific macros
> for lflow addition.
>
> This series is marked as an RFC mainly due to the potential difficulties
> this could cause with backports in the future. This series does not add
> any new functionality to the code, so it could be argued that this
> change is not worth it.
>
> A secondary reason this is marked as RFC is that, despite the
> simplification, it could be argued this removes some type-safety that
> was present in the old macros. For instance,
> ovn_lflow_add_with_dp_group() did not allow an ovn_datapath argument to
> be passed in. With the new setup, the ovn_datapath argument is required,
> and users have to set it NULL to get the desired effect. Similarly,
> ovn_lflow_add_drop_with_desc() was the only macro that allowed a flow
> description to be passed in. This implies that a flow description is
> only relevant for "drop" flows. However, the new set of macros allows
> flow descriptions to be passed in for any type of flow.
>
> Changes from v1:
> * The "where" argument is now part of the lflow_table_add_args
> structure.
> * I figured out a way to not have to repeat the struct initialization
> over and over for multiple macros. This is accomplished with the START
> and END macros that are added in this version.
> * This version no longer uses VFUNC. Instead, ovn_lflow_add() takes 7
> named arguments and "...". The way the helper macros are defined, we
> can simply sub __VA_ARGS__ into the macro definition and it works.
> * This version keeps the ovn_lflow_add_default_drop() macro.
> * The ovn_lflow_add_default_drop() change has been moved to patch 2. This
> way the series consists of two patches that add macros, followed by 6
> patches that remove macros.
> * This version uses double underscores for private functions.
> * This version changes the reasons for the patch being marked RFC in the
> cover letter.
>
>
Hi Mark,
thank you for the RFC. In general this seems to be a nice way to
do things. However I have a couple of small comments:
1) We should also get rid of ovn_lflow_add_default_drop() in favor
of doing "ovn_lflow_add(..., debug_drop_action(), ..., WITH_DESC(...))".
That was the plan anyway to actually have all drops with a comment,
this will also force any future contributor to use only a single macro.
2) After the use of "ovn_lflow_add()" a lot of calls now spans on multiple
lines we should re-align those if applicable.
3) If we want to make this happen it should be in 26.03, this makes it
way easier for us to backport later on.
> Mark Michelson (9):
> lflow-mgr: Use struct argument for lflow addition.
> lflows: Create new ovn_lflow_add_default_drop().
> lflows: Remove ovn_lflow_add_with_hint().
> lflows: Remove ovn_lflow_add_with_lport_and_hint().
> lflows: Remove ovn_lflow_metered() and ovn_lflow_add_with_hint__().
> lflows: Remove ovn_lflow_add_with_dp_group().
> lflows: Remove ovn_lflow_add_drop_with_desc().
> lflows: Remove ovn_lflow_add_drop_with_lport_hint_and_desc().
> lflows: Make non-struct version of lflow_table_add_lflow() private.
>
> northd/lflow-mgr.c | 39 +-
> northd/lflow-mgr.h | 129 ++--
> northd/northd.c | 1811 +++++++++++++++++++++++---------------------
> 3 files changed, 1047 insertions(+), 932 deletions(-)
>
> --
> 2.51.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
