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