diff mbox

[ovs-dev,v2,12/21] logical-fields: Beautify conntrack definitions.

Message ID 1470672872-19450-13-git-send-email-blp@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff Aug. 8, 2016, 4:14 p.m. UTC
The previous definitions, in my opinion, were ugly and also went beyond
80 columns.

This also adds a test that I generated based on the previous version, to
guard against regression.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovn/lib/logical-fields.c | 34 +++++++++++++++++++++-------------
 tests/ovn.at             | 15 +++++++++++++++
 2 files changed, 36 insertions(+), 13 deletions(-)

Comments

Ryan Moats Aug. 8, 2016, 5:21 p.m. UTC | #1
"dev" <dev-bounces@openvswitch.org> wrote on 08/08/2016 11:14:23 AM:

> From: Ben Pfaff <blp@ovn.org>
> To: dev@openvswitch.org
> Cc: Ben Pfaff <blp@ovn.org>
> Date: 08/08/2016 11:16 AM
> Subject: [ovs-dev] [PATCH v2 12/21] logical-fields: Beautify
> conntrack definitions.
> Sent by: "dev" <dev-bounces@openvswitch.org>
>
> The previous definitions, in my opinion, were ugly and also went beyond
> 80 columns.
>
> This also adds a test that I generated based on the previous version, to
> guard against regression.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---

I didn't get PS11 in my mailbox, so consider this an ack of
PS11 and PS12...

Acked-by: Ryan Moats <rmoats@us.ibm.com>
Ben Pfaff Aug. 8, 2016, 6:11 p.m. UTC | #2
On Mon, Aug 08, 2016 at 12:21:18PM -0500, Ryan Moats wrote:
> "dev" <dev-bounces@openvswitch.org> wrote on 08/08/2016 11:14:23 AM:
> 
> > From: Ben Pfaff <blp@ovn.org>
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff <blp@ovn.org>
> > Date: 08/08/2016 11:16 AM
> > Subject: [ovs-dev] [PATCH v2 12/21] logical-fields: Beautify
> > conntrack definitions.
> > Sent by: "dev" <dev-bounces@openvswitch.org>
> >
> > The previous definitions, in my opinion, were ugly and also went beyond
> > 80 columns.
> >
> > This also adds a test that I generated based on the previous version, to
> > guard against regression.
> >
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> 
> I didn't get PS11 in my mailbox, so consider this an ack of
> PS11 and PS12...
> 
> Acked-by: Ryan Moats <rmoats@us.ibm.com>

Thanks, I applied these to master up to this patch.
diff mbox

Patch

diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
index f79346d..6dbb4ae 100644
--- a/ovn/lib/logical-fields.c
+++ b/ovn/lib/logical-fields.c
@@ -93,19 +93,27 @@  ovn_init_symtab(struct shash *symtab)
     expr_symtab_add_field(symtab, "ct_mark", MFF_CT_MARK, NULL, false);
     expr_symtab_add_field(symtab, "ct_label", MFF_CT_LABEL, NULL, false);
     expr_symtab_add_field(symtab, "ct_state", MFF_CT_STATE, NULL, false);
-    char ct_state_str[16];
-    snprintf(ct_state_str, sizeof ct_state_str, "ct_state[%d]", CS_TRACKED_BIT);
-    expr_symtab_add_predicate(symtab, "ct.trk", ct_state_str);
-    snprintf(ct_state_str, sizeof ct_state_str, "ct_state[%d]", CS_NEW_BIT);
-    expr_symtab_add_subfield(symtab, "ct.new", "ct.trk", ct_state_str);
-    snprintf(ct_state_str, sizeof ct_state_str, "ct_state[%d]", CS_ESTABLISHED_BIT);
-    expr_symtab_add_subfield(symtab, "ct.est", "ct.trk", ct_state_str);
-    snprintf(ct_state_str, sizeof ct_state_str, "ct_state[%d]", CS_RELATED_BIT);
-    expr_symtab_add_subfield(symtab, "ct.rel", "ct.trk", ct_state_str);
-    snprintf(ct_state_str, sizeof ct_state_str, "ct_state[%d]", CS_REPLY_DIR_BIT);
-    expr_symtab_add_subfield(symtab, "ct.rpl", "ct.trk", ct_state_str);
-    snprintf(ct_state_str, sizeof ct_state_str, "ct_state[%d]", CS_INVALID_BIT);
-    expr_symtab_add_subfield(symtab, "ct.inv", "ct.trk", ct_state_str);
+
+    struct ct_bit {
+        const char *name;
+        int bit;
+    };
+    static const struct ct_bit bits[] = {
+        {"trk", CS_TRACKED_BIT},
+        {"new", CS_NEW_BIT},
+        {"est", CS_ESTABLISHED_BIT},
+        {"rel", CS_RELATED_BIT},
+        {"rpl", CS_REPLY_DIR_BIT},
+        {"inv", CS_INVALID_BIT},
+    };
+    for (const struct ct_bit *b = bits; b < &bits[ARRAY_SIZE(bits)]; b++) {
+        char *name = xasprintf("ct.%s", b->name);
+        char *expansion = xasprintf("ct_state[%d]", b->bit);
+        const char *prereqs = b->bit == CS_TRACKED_BIT ? NULL : "ct.trk";
+        expr_symtab_add_subfield(symtab, name, prereqs, expansion);
+        free(expansion);
+        free(name);
+    }
 
     /* Data fields. */
     expr_symtab_add_field(symtab, "eth.src", MFF_ETH_SRC, NULL, false);
diff --git a/tests/ovn.at b/tests/ovn.at
index e6d5af6..72868be 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -159,6 +159,21 @@  xxreg1 = NXM_NX_XXREG1
 ]])
 AT_CLEANUP
 
+dnl Check that the OVN conntrack field definitions are correct.
+AT_SETUP([ovn -- conntrack fields])
+AT_CHECK([ovstest test-ovn dump-symtab | grep ^ct | sort], [0],
+[[ct.est = ct_state[1]
+ct.inv = ct_state[4]
+ct.new = ct_state[0]
+ct.rel = ct_state[2]
+ct.rpl = ct_state[3]
+ct.trk = ct_state[5]
+ct_label = NXM_NX_CT_LABEL
+ct_mark = NXM_NX_CT_MARK
+ct_state = NXM_NX_CT_STATE
+]])
+AT_CLEANUP
+
 AT_SETUP([ovn -- expression parser])
 dnl For lines without =>, input and expected output are identical.
 dnl For lines with =>, input precedes => and expected output follows =>.