[ovs-dev,v3,13/16] ovn-controller: Maintain resource references for logical flows.

Message ID 1527874926-40450-14-git-send-email-hzhou8@ebay.com
State Superseded
Headers show
Series
  • ovn-controller incremental processing
Related show

Commit Message

Han Zhou June 1, 2018, 5:42 p.m.
This patch maintains the cross reference between logical flows and
the resources such as address sets and port groups that are used by
logical flows. This data will be needed in address set and port
group incremental processing.

Signed-off-by: Han Zhou <hzhou8@ebay.com>
---
 include/ovn/expr.h              |   5 +-
 ovn/controller/lflow.c          | 147 ++++++++++++++++++++++++++++++++++++++--
 ovn/controller/lflow.h          |  52 ++++++++++++++
 ovn/controller/ovn-controller.c |  14 ++--
 ovn/lib/actions.c               |   2 +-
 ovn/lib/expr.c                  |  21 ++++--
 ovn/utilities/ovn-trace.c       |   2 +-
 tests/test-ovn.c                |   7 +-
 8 files changed, 230 insertions(+), 20 deletions(-)

Patch

diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 3995e62..c0664ac 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -65,6 +65,7 @@  struct flow;
 struct ofpbuf;
 struct shash;
 struct simap;
+struct sset;
 
 /* "Measurement level" of a field.  See "Level of Measurement" in the large
  * comment on struct expr_symbol below for more information. */
@@ -383,10 +384,12 @@  void expr_format(const struct expr *, struct ds *);
 void expr_print(const struct expr *);
 struct expr *expr_parse(struct lexer *, const struct shash *symtab,
                         const struct shash *addr_sets,
-                        const struct shash *port_groups);
+                        const struct shash *port_groups,
+                        struct sset *addr_sets_ref);
 struct expr *expr_parse_string(const char *, const struct shash *symtab,
                                const struct shash *addr_sets,
                                const struct shash *port_groups,
+                               struct sset *addr_sets_ref,
                                char **errorp);
 
 struct expr *expr_clone(struct expr *);
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 9d8f693..a39e241 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -62,6 +62,7 @@  struct condition_aux {
 };
 
 static bool consider_logical_flow(struct ovn_desired_flow_table *flow_table,
+                                  struct lflow_resource_ref *lfrr,
                                   struct controller_ctx *ctx,
                                   const struct chassis_index *chassis_index,
                                   const struct sbrec_logical_flow *lflow,
@@ -135,9 +136,132 @@  is_switch(const struct sbrec_datapath_binding *ldp)
 
 }
 
+void
+lflow_resource_init(struct lflow_resource_ref *lfrr)
+{
+    hmap_init(&lfrr->ref_lflow_table);
+    hmap_init(&lfrr->lflow_ref_table);
+}
+
+void
+lflow_resource_destroy(struct lflow_resource_ref *lfrr)
+{
+    struct ref_lflow_node *rlfn, *rlfn_next;
+    HMAP_FOR_EACH_SAFE (rlfn, rlfn_next, node, &lfrr->ref_lflow_table) {
+        free(rlfn->ref_name);
+        struct lflow_ref_list_node *lrln, *next;
+        LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) {
+            ovs_list_remove(&lrln->ref_list);
+            ovs_list_remove(&lrln->lflow_list);
+            free(lrln);
+        }
+        hmap_remove(&lfrr->ref_lflow_table, &rlfn->node);
+        free(rlfn);
+    }
+    hmap_destroy(&lfrr->ref_lflow_table);
+
+    struct lflow_ref_node *lfrn, *lfrn_next;
+    HMAP_FOR_EACH_SAFE (lfrn, lfrn_next, node, &lfrr->lflow_ref_table) {
+        hmap_remove(&lfrr->lflow_ref_table, &lfrn->node);
+        free(lfrn);
+    }
+    hmap_destroy(&lfrr->lflow_ref_table);
+}
+
+void
+lflow_resource_clear(struct lflow_resource_ref *lfrr)
+{
+    lflow_resource_destroy(lfrr);
+    lflow_resource_init(lfrr);
+}
+
+static struct ref_lflow_node*
+ref_lflow_lookup(struct hmap *ref_lflow_table,
+                 enum ref_type type, const char *ref_name)
+{
+    struct ref_lflow_node *rlfn;
+
+    HMAP_FOR_EACH_WITH_HASH (rlfn, node, hash_string(ref_name, type),
+                             ref_lflow_table) {
+        if (rlfn->type == type && !strcmp(rlfn->ref_name, ref_name)) {
+            return rlfn;
+        }
+    }
+    return NULL;
+}
+
+static struct lflow_ref_node*
+lflow_ref_lookup(struct hmap *lflow_ref_table,
+                 const struct uuid *lflow_uuid)
+{
+    struct lflow_ref_node *lfrn;
+
+    HMAP_FOR_EACH_WITH_HASH (lfrn, node, uuid_hash(lflow_uuid),
+                             lflow_ref_table) {
+        if (uuid_equals(&lfrn->lflow_uuid, lflow_uuid)) {
+            return lfrn;
+        }
+    }
+    return NULL;
+}
+
+static void
+lflow_resource_add(struct lflow_resource_ref *lfrr, enum ref_type type,
+                   const char *ref_name, const struct uuid *lflow_uuid)
+{
+    struct ref_lflow_node *rlfn = ref_lflow_lookup(&lfrr->ref_lflow_table,
+                                                   type, ref_name);
+    if (!rlfn) {
+        rlfn = xzalloc(sizeof *rlfn);
+        rlfn->node.hash = hash_string(ref_name, type);
+        rlfn->type = type;
+        rlfn->ref_name = xstrdup(ref_name);
+        ovs_list_init(&rlfn->ref_lflow_head);
+        hmap_insert(&lfrr->ref_lflow_table, &rlfn->node, rlfn->node.hash);
+    }
+
+    struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table,
+                                                   lflow_uuid);
+    if (!lfrn) {
+        lfrn = xzalloc(sizeof *lfrn);
+        lfrn->node.hash = uuid_hash(lflow_uuid);
+        lfrn->lflow_uuid = *lflow_uuid;
+        ovs_list_init(&lfrn->lflow_ref_head);
+        hmap_insert(&lfrr->lflow_ref_table, &lfrn->node, lfrn->node.hash);
+    }
+
+    struct lflow_ref_list_node *lrln = xzalloc(sizeof *lrln);
+    lrln->type = type;
+    lrln->ref_name = xstrdup(ref_name);
+    lrln->lflow_uuid = *lflow_uuid;
+    ovs_list_push_back(&rlfn->ref_lflow_head, &lrln->ref_list);
+    ovs_list_push_back(&lfrn->lflow_ref_head, &lrln->lflow_list);
+}
+
+static void
+lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr,
+                            const struct uuid *lflow_uuid)
+{
+    struct lflow_ref_node *lfrn = lflow_ref_lookup(&lfrr->lflow_ref_table,
+                                                   lflow_uuid);
+    if (!lfrn) {
+        return;
+    }
+
+    hmap_remove(&lfrr->lflow_ref_table, &lfrn->node);
+    struct lflow_ref_list_node *lrln, *next;
+    LIST_FOR_EACH_SAFE (lrln, next, lflow_list, &lfrn->lflow_ref_head) {
+        ovs_list_remove(&lrln->ref_list);
+        ovs_list_remove(&lrln->lflow_list);
+        free(lrln);
+    }
+    free(lfrn);
+}
+
 /* Adds the logical flows from the Logical_Flow table to flow tables. */
 static void
 add_logical_flows(struct ovn_desired_flow_table *flow_table,
+                  struct lflow_resource_ref *lfrr,
                   struct controller_ctx *ctx,
                   const struct chassis_index *chassis_index,
                   const struct hmap *local_datapaths,
@@ -171,7 +295,7 @@  add_logical_flows(struct ovn_desired_flow_table *flow_table,
     nd_ra_opts_init(&nd_ra_opts);
 
     SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) {
-        if (!consider_logical_flow(flow_table, ctx, chassis_index,
+        if (!consider_logical_flow(flow_table, lfrr, ctx, chassis_index,
                                    lflow, local_datapaths,
                                    group_table, meter_table, chassis,
                                    &dhcp_opts, &dhcpv6_opts, &nd_ra_opts,
@@ -190,6 +314,7 @@  add_logical_flows(struct ovn_desired_flow_table *flow_table,
 
 bool
 lflow_handle_changed_flows(struct ovn_desired_flow_table *flow_table,
+                           struct lflow_resource_ref *lfrr,
                            struct controller_ctx *ctx,
                            const struct sbrec_chassis *chassis,
                            const struct chassis_index *chassis_index,
@@ -232,6 +357,8 @@  lflow_handle_changed_flows(struct ovn_desired_flow_table *flow_table,
             VLOG_DBG("handle deleted lflow "UUID_FMT,
                      UUID_ARGS(&lflow->header_.uuid));
             ofctrl_remove_flows(flow_table, &lflow->header_.uuid);
+            /* Delete entries from lflow resource reference. */
+            lflow_resource_destroy_lflow(lfrr, &lflow->header_.uuid);
         }
     }
     SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
@@ -243,10 +370,12 @@  lflow_handle_changed_flows(struct ovn_desired_flow_table *flow_table,
                 VLOG_DBG("handle updated lflow "UUID_FMT,
                          UUID_ARGS(&lflow->header_.uuid));
                 ofctrl_remove_flows(flow_table, &lflow->header_.uuid);
+                /* Delete entries from lflow resource reference. */
+                lflow_resource_destroy_lflow(lfrr, &lflow->header_.uuid);
             }
             VLOG_DBG("handle new lflow "UUID_FMT,
                      UUID_ARGS(&lflow->header_.uuid));
-            if (!consider_logical_flow(flow_table, ctx, chassis_index,
+            if (!consider_logical_flow(flow_table, lfrr, ctx, chassis_index,
                                        lflow, local_datapaths,
                                        group_table, meter_table, chassis,
                                        &dhcp_opts, &dhcpv6_opts, &nd_ra_opts,
@@ -276,6 +405,7 @@  update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
 
 static bool
 consider_logical_flow(struct ovn_desired_flow_table *flow_table,
+                      struct lflow_resource_ref *lfrr,
                       struct controller_ctx *ctx,
                       const struct chassis_index *chassis_index,
                       const struct sbrec_logical_flow *lflow,
@@ -349,8 +479,16 @@  consider_logical_flow(struct ovn_desired_flow_table *flow_table,
     struct hmap matches;
     struct expr *expr;
 
+    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
     expr = expr_parse_string(lflow->match, &symtab, addr_sets, port_groups,
-                             &error);
+                             &addr_sets_ref, &error);
+    const char *addr_set_name;
+    SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
+        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
+                           &lflow->header_.uuid);
+    }
+    sset_destroy(&addr_sets_ref);
+
     if (!error) {
         if (prereqs) {
             expr = expr_combine(EXPR_T_AND, expr, prereqs);
@@ -544,6 +682,7 @@  add_neighbor_flows(struct ovn_desired_flow_table *flow_table,
  * into OpenFlow flows.  See ovn-architecture(7) for more information. */
 void
 lflow_run(struct ovn_desired_flow_table *flow_table,
+          struct lflow_resource_ref *lfrr,
           struct controller_ctx *ctx,
           const struct sbrec_chassis *chassis,
           const struct chassis_index *chassis_index,
@@ -558,7 +697,7 @@  lflow_run(struct ovn_desired_flow_table *flow_table,
 {
     COVERAGE_INC(lflow_run);
 
-    add_logical_flows(flow_table, ctx, chassis_index, local_datapaths,
+    add_logical_flows(flow_table, lfrr, ctx, chassis_index, local_datapaths,
                       group_table, meter_table, chassis, addr_sets,
                       port_groups, active_tunnels, local_lport_ids,
                       conj_id_ofs);
diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
index d407c2c..bc6c7ac 100644
--- a/ovn/controller/lflow.h
+++ b/ovn/controller/lflow.h
@@ -34,12 +34,16 @@ 
  */
 
 #include <stdint.h>
+#include "openvswitch/hmap.h"
+#include "openvswitch/uuid.h"
+#include "openvswitch/list.h"
 
 struct chassis_index;
 struct controller_ctx;
 struct ovn_extend_table;
 struct ovn_desired_flow_table;
 struct hmap;
+struct hmap_node;
 struct sbrec_chassis;
 struct simap;
 struct sset;
@@ -62,8 +66,55 @@  struct uuid;
 /* The number of tables for the ingress and egress pipelines. */
 #define LOG_PIPELINE_LEN 24
 
+enum ref_type {
+    REF_TYPE_ADDRSET
+};
+
+/* Maintains the relationship for a pair of named resource and
+ * a lflow, indexed by both ref_lflow_table and lflow_ref_table. */
+struct lflow_ref_list_node {
+    struct ovs_list lflow_list; /* list for same lflow */
+    struct ovs_list ref_list; /* list for same ref */
+    enum ref_type type;
+    char *ref_name;
+    struct uuid lflow_uuid;
+};
+
+struct ref_lflow_node {
+    struct hmap_node node;
+    enum ref_type type; /* key */
+    char *ref_name; /* key */
+    struct ovs_list ref_lflow_head;
+};
+
+struct lflow_ref_node {
+    struct hmap_node node;
+    struct uuid lflow_uuid; /* key */
+    struct ovs_list lflow_ref_head;
+};
+
+struct lflow_resource_ref {
+    /* A map from a referenced resource type & name (e.g. address_set AS1)
+     * to a list of lflows that are referencing the named resource. Data
+     * type of each node in this hmap is struct ref_lflow_node. The
+     * ref_lflow_head in each node points to a list of
+     * lflow_ref_list_node.ref_list. */
+    struct hmap ref_lflow_table;
+
+    /* A map from a lflow uuid to a list of named resources that are
+     * referenced by the lflow. Data type of each node in this hmap is
+     * struct lflow_ref_node. The lflow_ref_head in each node points to
+     * a list of lflow_ref_list_node.lflow_list. */
+    struct hmap lflow_ref_table;
+};
+
+void lflow_resource_init(struct lflow_resource_ref *);
+void lflow_resource_destroy(struct lflow_resource_ref *);
+void lflow_resource_clear(struct lflow_resource_ref *);
+
 void lflow_init(void);
 void lflow_run(struct ovn_desired_flow_table *,
+               struct lflow_resource_ref *,
                struct controller_ctx *,
                const struct sbrec_chassis *chassis,
                const struct chassis_index *,
@@ -76,6 +127,7 @@  void lflow_run(struct ovn_desired_flow_table *,
                struct sset *local_lport_ids,
                uint32_t *conj_id_ofs);
 bool lflow_handle_changed_flows(struct ovn_desired_flow_table *,
+                                struct lflow_resource_ref *,
                                 struct controller_ctx *ctx,
                                 const struct sbrec_chassis *chassis,
                                 const struct chassis_index *chassis_index,
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 8b0cd46..7d0bde5 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -831,6 +831,8 @@  struct ed_type_flow_output {
     struct ovn_extend_table meter_table;
     /* conjunction id offset */
     uint32_t conj_id_ofs;
+    /* lflow resource cross reference */
+    struct lflow_resource_ref lflow_resource_ref;
 };
 
 static void
@@ -842,6 +844,7 @@  en_flow_output_init(struct engine_node *node)
     ovn_extend_table_init(&data->group_table);
     ovn_extend_table_init(&data->meter_table);
     data->conj_id_ofs = 1;
+    lflow_resource_init(&data->lflow_resource_ref);
 }
 
 static void
@@ -852,6 +855,7 @@  en_flow_output_cleanup(struct engine_node *node)
     ovn_desired_flow_table_destroy(&data->flow_table);
     ovn_extend_table_destroy(&data->group_table);
     ovn_extend_table_destroy(&data->meter_table);
+    lflow_resource_destroy(&data->lflow_resource_ref);
 }
 
 static void
@@ -890,6 +894,7 @@  en_flow_output_run(struct engine_node *node)
     struct ovn_extend_table *group_table = &fo->group_table;
     struct ovn_extend_table *meter_table = &fo->meter_table;
     uint32_t *conj_id_ofs = &fo->conj_id_ofs;
+    struct lflow_resource_ref *lfrr = &fo->lflow_resource_ref;
 
     static bool first_run = true;
     if (first_run) {
@@ -898,12 +903,12 @@  en_flow_output_run(struct engine_node *node)
         ovn_desired_flow_table_clear(flow_table);
         ovn_extend_table_clear(group_table, false /* desired */);
         ovn_extend_table_clear(meter_table, false /* desired */);
+        lflow_resource_clear(lfrr);
     }
 
     *conj_id_ofs = 1;
-    lflow_run(flow_table, ctx, chassis,
-              chassis_index, local_datapaths, group_table,
-              meter_table, addr_sets, port_groups, active_tunnels,
+    lflow_run(flow_table, lfrr, ctx, chassis, chassis_index, local_datapaths,
+              group_table, meter_table, addr_sets, port_groups, active_tunnels,
               local_lport_ids, conj_id_ofs);
 
     enum mf_field_id mff_ovn_geneve = ofctrl_get_mf_field_id();
@@ -950,8 +955,9 @@  flow_output_sb_logical_flow_handler(struct engine_node *node)
     struct ovn_extend_table *group_table = &fo->group_table;
     struct ovn_extend_table *meter_table = &fo->meter_table;
     uint32_t *conj_id_ofs = &fo->conj_id_ofs;
+    struct lflow_resource_ref *lfrr = &fo->lflow_resource_ref;
 
-    bool handled = lflow_handle_changed_flows(flow_table, ctx, chassis,
+    bool handled = lflow_handle_changed_flows(flow_table, lfrr, ctx, chassis,
               chassis_index, local_datapaths, group_table, meter_table,
               addr_sets, port_groups, active_tunnels, local_lport_ids,
               conj_id_ofs);
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 20c8267..1cb9bed 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -234,7 +234,7 @@  add_prerequisite(struct action_context *ctx, const char *prerequisite)
     struct expr *expr;
     char *error;
 
-    expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL,
+    expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, NULL,
                              &error);
     ovs_assert(!error);
     ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr);
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 148ac86..13c81c8 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -465,6 +465,7 @@  struct expr_context {
     const struct shash *symtab;    /* Symbol table. */
     const struct shash *addr_sets; /* Address set table. */
     const struct shash *port_groups; /* Port group table. */
+    struct sset *addr_sets_ref;       /* The set of address set referenced. */
     bool not;                    /* True inside odd number of NOT operators. */
 };
 
@@ -727,6 +728,10 @@  static bool
 parse_addr_sets(struct expr_context *ctx, struct expr_constant_set *cs,
                 size_t *allocated_values)
 {
+    if (ctx->addr_sets_ref) {
+        sset_add(ctx->addr_sets_ref, ctx->lexer->token.s);
+    }
+
     struct expr_constant_set *addr_sets
         = (ctx->addr_sets
            ? shash_find_data(ctx->addr_sets, ctx->lexer->token.s)
@@ -1261,12 +1266,14 @@  expr_parse__(struct expr_context *ctx)
 struct expr *
 expr_parse(struct lexer *lexer, const struct shash *symtab,
            const struct shash *addr_sets,
-           const struct shash *port_groups)
+           const struct shash *port_groups,
+           struct sset *addr_sets_ref)
 {
     struct expr_context ctx = { .lexer = lexer,
                                 .symtab = symtab,
                                 .addr_sets = addr_sets,
-                                .port_groups = port_groups };
+                                .port_groups = port_groups,
+                                .addr_sets_ref = addr_sets_ref };
     return lexer->error ? NULL : expr_parse__(&ctx);
 }
 
@@ -1280,13 +1287,15 @@  struct expr *
 expr_parse_string(const char *s, const struct shash *symtab,
                   const struct shash *addr_sets,
                   const struct shash *port_groups,
+                  struct sset *addr_sets_ref,
                   char **errorp)
 {
     struct lexer lexer;
 
     lexer_init(&lexer, s);
     lexer_get(&lexer);
-    struct expr *expr = expr_parse(&lexer, symtab, addr_sets, port_groups);
+    struct expr *expr = expr_parse(&lexer, symtab, addr_sets, port_groups,
+                                   addr_sets_ref);
     lexer_force_end(&lexer);
     *errorp = lexer_steal_error(&lexer);
     if (*errorp) {
@@ -1510,7 +1519,7 @@  expr_get_level(const struct expr *expr)
 static enum expr_level
 expr_parse_level(const char *s, const struct shash *symtab, char **errorp)
 {
-    struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, errorp);
+    struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, NULL, errorp);
     enum expr_level level = expr ? expr_get_level(expr) : EXPR_L_NOMINAL;
     expr_destroy(expr);
     return level;
@@ -1668,7 +1677,7 @@  parse_and_annotate(const char *s, const struct shash *symtab,
     char *error;
     struct expr *expr;
 
-    expr = expr_parse_string(s, symtab, NULL, NULL, &error);
+    expr = expr_parse_string(s, symtab, NULL, NULL, NULL, &error);
     if (expr) {
         expr = expr_annotate_(expr, symtab, nesting, &error);
     }
@@ -3392,7 +3401,7 @@  expr_parse_microflow(const char *s, const struct shash *symtab,
     lexer_init(&lexer, s);
     lexer_get(&lexer);
 
-    struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups);
+    struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups, NULL);
     lexer_force_end(&lexer);
 
     if (e) {
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 1fd48f2..39fa14c 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -813,7 +813,7 @@  read_flows(void)
         char *error;
         struct expr *match;
         match = expr_parse_string(sblf->match, &symtab, &address_sets,
-                                  &port_groups, &error);
+                                  &port_groups, NULL, &error);
         if (error) {
             VLOG_WARN("%s: parsing expression failed (%s)",
                       sblf->match, error);
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index d4a5d59..ce4e8e3 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -281,7 +281,7 @@  test_parse_expr__(int steps)
         char *error;
 
         expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets,
-                                 &port_groups, &error);
+                                 &port_groups, NULL, &error);
         if (!error && steps > 0) {
             expr = expr_annotate(expr, &symtab, &error);
         }
@@ -405,7 +405,8 @@  test_evaluate_expr(struct ovs_cmdl_context *ctx)
     while (!ds_get_test_line(&input, stdin)) {
         struct expr *expr;
 
-        expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, &error);
+        expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, NULL,
+                                 &error);
         if (!error) {
             expr = expr_annotate(expr, &symtab, &error);
         }
@@ -880,7 +881,7 @@  test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
 
             char *error;
             modified = expr_parse_string(ds_cstr(&s), symtab, NULL,
-                                         NULL, &error);
+                                         NULL, NULL, &error);
             if (error) {
                 fprintf(stderr, "%s fails to parse (%s)\n",
                         ds_cstr(&s), error);