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

Dumitru Ceara dceara at redhat.com
Wed Sep 16 10:26:13 UTC 2020


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> 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>
>> Signed-off-by: Han Zhou <hzhou at ovn.org>
>>
> 
> Thanks for the fix. The name was misleading.
> 
> Acked-by: Numan Siddique <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.

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
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list