[ovs-dev] [PATCH ovn] lflow.c: Rename function convert_acts_to_expr to convert_match_to_expr.

Han Zhou hzhou at ovn.org
Wed Sep 16 16:14:37 UTC 2020


On Wed, Sep 16, 2020 at 7:28 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 9/16/20 4:04 PM, Numan Siddique wrote:
> >
> >
> >
> > On Wed, Sep 16, 2020 at 3:56 PM Dumitru Ceara <dceara at redhat.com
> > <mailto:dceara at redhat.com>> wrote:
> >
> >     On 9/16/20 8:11 AM, Numan Siddique wrote:
> >     > On Wed, Sep 16, 2020 at 9:18 AM Han Zhou <hzhou at ovn.org
> >     <mailto:hzhou at ovn.org>> wrote:
> >     >
> >     >> The name was misleading because it in fact parses lflow match
instead
> >     >> of actions.
> >     >>
> >     >> Fixes: 1213bc8270 ("ovn-controller: Cache logical flow expr
> >     matches.")
> >     >> Cc: Numan Siddique <numans at ovn.org <mailto:numans at ovn.org>>
> >     >> Signed-off-by: Han Zhou <hzhou at ovn.org <mailto:hzhou at ovn.org>>
> >     >>
> >     >
> >     > Thanks for the fix. The name was misleading.
> >     >
> >     > Acked-by: Numan Siddique <numans at ovn.org <mailto:numans at ovn.org>>
> >     >
> >     > Numan
> >     >
> >     > ---
> >     >>  controller/lflow.c | 16 ++++++++--------
> >     >>  1 file changed, 8 insertions(+), 8 deletions(-)
> >     >>
> >     >> diff --git a/controller/lflow.c b/controller/lflow.c
> >     >> index e785866..9a96aac 100644
> >     >> --- a/controller/lflow.c
> >     >> +++ b/controller/lflow.c
> >     >> @@ -720,15 +720,15 @@ add_matches_to_flow_table(const struct
> >     >> sbrec_logical_flow *lflow,
> >     >>      ofpbuf_uninit(&ofpacts);
> >     >>  }
> >     >>
> >     >> -/* Converts the actions and returns the simplified expre tree.
> >     >> +/* Converts the match and returns the simplified expre tree.
> >     >>   * The caller should evaluate the conditions and normalize the
> >     >>   * expr tree. */
> >     >>  static struct expr *
> >     >> -convert_acts_to_expr(const struct sbrec_logical_flow *lflow,
> >     >> -                     struct expr *prereqs,
> >     >> -                     struct lflow_ctx_in *l_ctx_in,
> >     >> -                     struct lflow_ctx_out *l_ctx_out,
> >     >> -                     bool *pg_addr_set_ref, char **errorp)
> >     >> +convert_match_to_expr(const struct sbrec_logical_flow *lflow,
> >     >> +                      struct expr *prereqs,
> >     >> +                      struct lflow_ctx_in *l_ctx_in,
> >     >> +                      struct lflow_ctx_out *l_ctx_out,
> >     >> +                      bool *pg_addr_set_ref, char **errorp)
> >
> >     Hi Han, Numan,
> >
> >     To be honest, we might be able to do better. We still have to look
at
> >     the implementation of convert_match_to_expr() to see what it uses
from
> >     l_ctx_in and what side effects it has on l_ctx_out. Right now it:
> >
> >     - parses lflow->match
> >     - uses lflow->header_.uuid
> >     - uses l_ctx_in->port_groups/address_sets
> >     - it modifies l_ctx_out->lfrr
> >     - it also sets pg_addr_set_ref and errorp
> >
> >     I'm finding it hard to track all the side effects without scanning
every
> >     line of the function.
> >
> >     Can't we refactor the function and just pass it the in/out
arguments it
> >     needs?
> >
> >     I know there was an effort a while ago to put all "input"/"output"
> >     parameters in in/out "context" structures, but passing the large
> >     contexts in a lot of functions is almost the same thing as having
the
> >     contexts stored as global variables.
> >
> >
> >
> > Hi Dumitru,
> >
>
> Hi Numan,
>
> > Well, I'm not really sure if it's a bad programming choice or not.
> > I personally would like to see less arguments passed to the functions
> > and hence prefer a context variable to be passed. We adopted the similar
> > function arguments for the I-P improvement patch series.
> > One such example -
> > https://github.com/ovn-org/ovn/blob/master/controller/binding.c#L509
> > where we just use the one variable in this function from the
> > binding_ctx_out.
> >
>
> I'm not sure either, but I do find it harder to get the gist of what a
> function does (or should do) without reading every line of the function.
>
I think either approach has its pros and cons here. With ctx wrapping the
args, the function prototypes are concise. However, it does make the
dependency less obvious. Initially when the parameter was converted to ctx,
it only replaces the places when all the members are used. Now it seems it
is easily passed to a static function that only uses some of them, thus
hiding the real dependency. So, I tend to agree with Dumitru in this
specific case that it may be better to unwrap the ctx structure because it
is helpful for tracking the I-P dependency. If we make this change, it
should belong to a separate patch, though.

> > I tried to see if there are any coding guidelines (in OVS/OVN) for
> > passing arguments
> > to the functions and what is the preferred way. I couldn't find any.
> >
>
> There are some general guidelines:
>
>
https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#functions
>
> > If you think passing specific arguments would be better, I'm fine with
it.
> >
> > But I think this patch is addressing a specific case and I think it's
> > better to
> > address your concerns in a separate patch. I'm fine with this patch AS
IS.
> >
>
> There are a few other minor comments related to incorrect indentation
> that should be fixed before this gets merged.

Thanks for spotting this. I sent v2 for the indentation. Please take a look.

>
> > It's up to Han if he wants to spin up another patch to change the
> > function arguments.
> > Otherwise, I request you to submit a follow patch for this.
> >
>
> I'll do my best :)
>
> > Thanks
> > Numan
> >
>
> Thanks,
> Dumitru
>
> >
> >     Also, now that we're fixing this function we should also remove this
> >     line:
> >
> >
https://github.com/ovn-org/ovn/blob/7c6009af2c0766b38c56147c21d0b9cb887389e3/controller/lflow.c#L757
> >
> >     >>  {
> >     >>      struct sset addr_sets_ref =
SSET_INITIALIZER(&addr_sets_ref);
> >     >>      struct sset port_groups_ref =
> >     SSET_INITIALIZER(&port_groups_ref);
> >     >> @@ -861,7 +861,7 @@ consider_logical_flow(const struct
> >     sbrec_logical_flow
> >     >> *lflow,
> >     >>      struct expr *expr = NULL;
> >     >>      if (!l_ctx_out->lflow_cache_map) {
> >     >>          /* Caching is disabled. */
> >     >> -        expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in,
> >     >> +        expr = convert_match_to_expr(lflow, prereqs, l_ctx_in,
> >     >>                                      l_ctx_out, NULL, &error);
> >
> >     Indentation is wrong here.
> >
> >     >>          if (error) {
> >     >>              expr_destroy(prereqs);
> >     >> @@ -924,7 +924,7 @@ consider_logical_flow(const struct
> >     sbrec_logical_flow
> >     >> *lflow,
> >     >>
> >     >>      bool pg_addr_set_ref = false;
> >     >>      if (!expr) {
> >     >> -        expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in,
> >     l_ctx_out,
> >     >> +        expr = convert_match_to_expr(lflow, prereqs, l_ctx_in,
> >     l_ctx_out,
> >     >>                                      &pg_addr_set_ref, &error);
> >
> >     Indentation is wrong here.
> >
> >     Regards,
> >     Dumitru
> >
> >     >>          if (error) {
> >     >>              expr_destroy(prereqs);
> >     >> --
> >     >> 2.1.0
> >     >>
> >     >> _______________________________________________
> >     >> dev mailing list
> >     >> dev at openvswitch.org <mailto:dev at openvswitch.org>
> >     >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >     >>
> >     >>
> >     > _______________________________________________
> >     > dev mailing list
> >     > dev at openvswitch.org <mailto:dev at openvswitch.org>
> >     > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >     >
> >
> >     _______________________________________________
> >     dev mailing list
> >     dev at openvswitch.org <mailto:dev at openvswitch.org>
> >     https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>


More information about the dev mailing list