On 9/16/20 4:04 PM, Numan Siddique wrote: > > > > On Wed, Sep 16, 2020 at 3:56 PM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: > > On 9/16/20 8:11 AM, Numan Siddique wrote: > > On Wed, Sep 16, 2020 at 9:18 AM Han Zhou <[email protected] > <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> > >> Signed-off-by: Han Zhou <[email protected] <mailto:[email protected]>> > >> > > > > Thanks for the fix. The name was misleading. > > > > Acked-by: Numan Siddique <[email protected] <mailto:[email protected]>> > > > > 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 > >> [email protected] <mailto:[email protected]> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > _______________________________________________ > > dev mailing list > > [email protected] <mailto:[email protected]> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
