[ovs-dev] [PATCH ovn] lflow.c: Rename function convert_acts_to_expr to convert_match_to_expr.
Dumitru Ceara
dceara at redhat.com
Wed Sep 16 14:28:42 UTC 2020
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 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.
> 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