[ovs-dev] [PATCH ovn v2] lflow.c: Refactor function convert_match_to_expr.
Mark Michelson
mmichels at redhat.com
Thu Oct 1 14:17:10 UTC 2020
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