[ovs-dev] [PATCH ovn] lflow.c: Refactor function convert_match_to_expr.

Dumitru Ceara dceara at redhat.com
Thu Oct 1 10:37:46 UTC 2020


On 10/1/20 12:19 PM, Mark Gray wrote:
> On 30/09/2020 11:56, Dumitru Ceara wrote:
>> To make it easier to understand what the function does we now explicitly
>> pass the input/output arguments instead of the complete
>> lflow_ctx_in/lflow_ctx_out context.
> 
> I think this is a good idea as it should make it easier to understand
> what is going on. Creating the contexts was a good first step but
> refactoring like this will help readability.
> 
> Thanks for the patch.
> 

Thanks for the review!

>>
>> Also, change the error handling to avoid forcing the caller to supply
>> (and free) an error string pointer.
>>
>> CC: Han Zhou <hzhou at ovn.org>
>> CC: Numan Siddique <numans at ovn.org>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>> ---
>>  controller/lflow.c | 61 ++++++++++++++++++++++++------------------------------
>>  1 file changed, 27 insertions(+), 34 deletions(-)
>>
>> diff --git a/controller/lflow.c b/controller/lflow.c
>> index 4d71dfd..d9d1477 100644
>> --- a/controller/lflow.c
>> +++ b/controller/lflow.c
>> @@ -761,35 +761,41 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> 
> Could you fix the comment on this aswell? There is a typo s/expre/expr

Ack.

>>  static struct expr *
>>  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)
>> +                      const struct shash *addr_sets,
>> +                      const struct shash *port_groups,
>> +                      struct lflow_resource_ref *lfrr,
>> +                      bool *pg_addr_set_ref)
>>  {
>>      struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
>>      struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
>> -    char *error;
>> +    char *error = NULL;
>>  
>> -    struct expr *e = expr_parse_string(lflow->match, &symtab,
>> -                                       l_ctx_in->addr_sets,
>> -                                       l_ctx_in->port_groups, &addr_sets_ref,
>> +    struct expr *e = expr_parse_string(lflow->match, &symtab, addr_sets,
>> +                                       port_groups, &addr_sets_ref,
>>                                         &port_groups_ref,
>>                                         lflow->logical_datapath->tunnel_key,
>>                                         &error);
> 
> The comment on this function (and expr_parse() is wrong as it specifies
> that it only used addr_sets to generate the expression. Can you update
> both please?
> 

Ack, and we should also update the comment for expr_parse_microflow().

>>      const char *addr_set_name;
>>      SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
>> -        lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name,
>> +        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
>>                             &lflow->header_.uuid);
>>      }
>>      const char *port_group_name;
>>      SSET_FOR_EACH (port_group_name, &port_groups_ref) {
>> -        lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
>> -                           port_group_name, &lflow->header_.uuid);
>> +        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
>> +                           &lflow->header_.uuid);
>> +    }
>> +
>> +    if (pg_addr_set_ref) {
>> +        *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) ||
>> +                            !sset_is_empty(&addr_sets_ref));
>>      }
>> +    sset_destroy(&addr_sets_ref);
>> +    sset_destroy(&port_groups_ref);
>>  
>>      if (!error) {
>>          if (prereqs) {
>>              e = expr_combine(EXPR_T_AND, e, prereqs);
>> -            prereqs = NULL;
>>          }
>>          e = expr_annotate(e, &symtab, &error);
>>      }
>> @@ -797,24 +803,11 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow,
>>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>>          VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s",
>>                      lflow->match, error);
>> -        sset_destroy(&addr_sets_ref);
>> -        sset_destroy(&port_groups_ref);
>> -        *errorp = error;
>> +        free(error);
> 
> On failure do we need to undo the flow_resource_add()?
> 

We didn't do before and I think it's fine to leave as is because failure
to parse will trigger a recompute of the I-P engine and call
en_flow_output_run() which should cleanup the resource refs.

Unfortunately I don't see an easier way to do cleanup.

I'll send a v2 soon fixing the points above.

Thanks,
Dumitru

>>          return NULL;
>>      }
>>  
>> -    e = expr_simplify(e);
>> -
>> -    if (pg_addr_set_ref) {
>> -        *pg_addr_set_ref = (!sset_is_empty(&port_groups_ref) ||
>> -                            !sset_is_empty(&addr_sets_ref));
>> -    }
>> -
>> -    sset_destroy(&addr_sets_ref);
>> -    sset_destroy(&port_groups_ref);
>> -
>> -    *errorp = NULL;
>> -    return e;
>> +    return expr_simplify(e);
>>  }
>>  
>>  static bool
>> @@ -896,13 +889,13 @@ 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_match_to_expr(lflow, prereqs, l_ctx_in,
>> -                                     l_ctx_out, NULL, &error);
>> -        if (error) {
>> +        expr = convert_match_to_expr(lflow, prereqs, l_ctx_in->addr_sets,
>> +                                     l_ctx_in->port_groups, l_ctx_out->lfrr,
>> +                                     NULL);
>> +        if (!expr) {
>>              expr_destroy(prereqs);
>>              ovnacts_free(ovnacts.data, ovnacts.size);
>>              ofpbuf_uninit(&ovnacts);
>> -            free(error);
>>              return true;
>>          }
>>  
>> @@ -959,13 +952,13 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>>  
>>      bool pg_addr_set_ref = false;
>>      if (!expr) {
>> -        expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out,
>> -                                     &pg_addr_set_ref, &error);
>> -        if (error) {
>> +        expr = convert_match_to_expr(lflow, prereqs, l_ctx_in->addr_sets,
>> +                                     l_ctx_in->port_groups, l_ctx_out->lfrr,
>> +                                     &pg_addr_set_ref);
>> +        if (!expr) {
>>              expr_destroy(prereqs);
>>              ovnacts_free(ovnacts.data, ovnacts.size);
>>              ofpbuf_uninit(&ovnacts);
>> -            free(error);
>>              return true;
>>          }
>>      } else {
>>
> 



More information about the dev mailing list