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

Dumitru Ceara dceara at redhat.com
Thu Oct 1 11:22:13 UTC 2020


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



More information about the dev mailing list