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

Wed Sep 16 14:04:20 UTC 2020

On Wed, Sep 16, 2020 at 3:56 PM Dumitru Ceara <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> 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>
> >> Signed-off-by: Han Zhou <hzhou at ovn.org>
> >>
> >
> > Thanks for the fix. The name was misleading.
> >
> > Acked-by: Numan Siddique <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,

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 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.

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.

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.

Thanks
Numan

> 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
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >>
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>