[ovs-dev] [PATCH ovn] lflow.c: Rename function convert_acts_to_expr to convert_match_to_expr.

Dumitru Ceara dceara at redhat.com
Thu Sep 17 07:26:20 UTC 2020


On 9/16/20 6:14 PM, Han Zhou wrote:
> 
> 
> On Wed, Sep 16, 2020 at 7:28 AM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
>>
>> 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>
>> > <mailto: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>
>> >     <mailto: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>
> <mailto:numans at ovn.org <mailto:numans at ovn.org>>>
>> >     >> Signed-off-by: Han Zhou <hzhou at ovn.org <mailto:hzhou at ovn.org>
> <mailto: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> <mailto: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 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



More information about the dev mailing list