On 9/16/20 6:14 PM, Han Zhou wrote: > > > On Wed, Sep 16, 2020 at 7:28 AM Dumitru Ceara <[email protected] > <mailto:[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]> >> > <mailto:[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]> >> > <mailto:[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]> > <mailto:[email protected] <mailto:[email protected]>>> >> > >> Signed-off-by: Han Zhou <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>>> >> > >> >> > > >> > > Thanks for the fix. The name was misleading. >> > > >> > > Acked-by: Numan Siddique <[email protected] > <mailto:[email protected]> <mailto:[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. >
Hi Han, I acked v2 and I'll try to follow up on this part in the coming weeks. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
