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

Mark Gray mark.d.gray at redhat.com
Thu Oct 1 10:19:23 UTC 2020


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.

> 
> 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
>  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?

>      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()?

>          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