[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