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

Han Zhou hzhou at ovn.org
Thu Oct 1 23:59:40 UTC 2020


Thanks Dumitru and Mark. I applied this to master.

Han

On Thu, Oct 1, 2020 at 7:17 AM Mark Michelson <mmichels at redhat.com> wrote:

> Acked-by: Mark Michelson <mmichels at redhat.com>
>
> On 10/1/20 7:22 AM, 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.
> >
> > Also:
> > - change the error handling to avoid forcing the caller to supply
> >    (and free) an error string pointer.
> > - update function comments in expr.c to mention that port_groups sets
> >    are also used for parsing.
> >
> > CC: Han Zhou <hzhou at ovn.org>
> > CC: Numan Siddique <numans at ovn.org>
> > Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> > ---
> > v2: Fix typos and comments as suggested by Mark Gray.
> > ---
> >   controller/lflow.c | 68
> +++++++++++++++++++++++++-----------------------------
> >   lib/expr.c         | 18 +++++++--------
> >   2 files changed, 40 insertions(+), 46 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 4d71dfd..f631679 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -755,41 +755,48 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
> >       ofpbuf_uninit(&ofpacts);
> >   }
> >
> > -/* Converts the match and returns the simplified expre tree.
> > - * The caller should evaluate the conditions and normalize the
> > - * expr tree. */
> > +/* Converts the match and returns the simplified expr tree.
> > + *
> > + * The caller should evaluate the conditions and normalize the expr
> tree.
> > + */
> >   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);
> >       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 +804,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);
> >           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 +890,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 +953,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 {
> > diff --git a/lib/expr.c b/lib/expr.c
> > index f6b22cf..acb1f3a 100644
> > --- a/lib/expr.c
> > +++ b/lib/expr.c
> > @@ -1317,9 +1317,9 @@ expr_parse__(struct expr_context *ctx)
> >   }
> >
> >   /* Parses an expression from 'lexer' using the symbols in 'symtab' and
> > - * address set table in 'addr_sets'.  If successful, returns the new
> > - * expression; on failure, returns NULL.  Returns nonnull if and only if
> > - * lexer->error is NULL. */
> > + * address set table in 'addr_sets' and 'port_groups'.  If successful,
> returns
> > + * the new expression; on failure, returns NULL.  Returns nonnull if
> and only
> > + * if lexer->error is NULL. */
> >   struct expr *
> >   expr_parse(struct lexer *lexer, const struct shash *symtab,
> >              const struct shash *addr_sets,
> > @@ -1339,9 +1339,9 @@ expr_parse(struct lexer *lexer, const struct shash
> *symtab,
> >   }
> >
> >   /* Parses the expression in 's' using the symbols in 'symtab' and
> > - * address set table in 'addr_sets'.  If successful, returns the new
> > - * expression and sets '*errorp' to NULL.  On failure, returns NULL and
> > - * sets '*errorp' to an explanatory error message.  The caller must
> > + * address set table in 'addr_sets' and 'port_groups'.  If successful,
> returns
> > + * the new expression and sets '*errorp' to NULL.  On failure, returns
> NULL
> > + * and sets '*errorp' to an explanatory error message.  The caller must
> >    * eventually free the returned expression (with expr_destroy()) or
> >    * error (with free()). */
> >   struct expr *
> > @@ -3481,9 +3481,9 @@ expr_parse_microflow__(struct lexer *lexer,
> >   }
> >
> >   /* Parses 's' as a microflow, using symbols from 'symtab', address set
> > - * table from 'addr_sets', and looking up port numbers using
> 'lookup_port'
> > - * and 'aux'.  On success, stores the result in 'uflow' and returns
> > - * NULL, otherwise zeros 'uflow' and returns an error message that the
> > + * table from 'addr_sets' and 'port_groups', and looking up port
> numbers using
> > + * 'lookup_port' and 'aux'.  On success, stores the result in 'uflow'
> and
> > + * returns NULL, otherwise zeros 'uflow' and returns an error message
> that the
> >    * caller must free().
> >    *
> >    * A "microflow" is a description of a single stream of packets, such
> as half a
> >
>
>


More information about the dev mailing list