On Wed, Sep 16, 2020 at 7:28 AM Dumitru Ceara <[email protected]> wrote: > > 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 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 > > >> [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
