[ovs-dev] [PATCH 2/2] nicira-ext: New Nicira vendor action NXAST_NOTE.

Ben Pfaff blp at nicira.com
Sat Nov 13 00:29:04 UTC 2010


On Fri, Nov 12, 2010 at 03:28:00PM -0800, Justin Pettit wrote:
> On Nov 10, 2010, at 4:28 PM, Ben Pfaff wrote:
> 
> > +/* Action structure for NXAST_NOTE.
> > + *
> > + * This action has no effect.  It is variable length.  The switch does not
> > + * attempt to interpret the user-defined 'note' data in any way.  A controller
> > + * can use this action to attach arbitrary metadata to a flow.
> > + */
> > +struct nx_action_note {
> > +    uint16_t type;                  /* OFPAT_VENDOR. */
> > +    uint16_t len;                   /* At least 16. */
> > +    uint32_t vendor;                /* NX_VENDOR_ID. */
> > +    uint16_t subtype;               /* NXAST_NOTE. */
> > +    uint8_t note[6];                /* Start of user-defined data. */
> > +    /* Possibly followed by additional user-defined data. */
> > +};
> 
> It would be good to mention that the note needs to be a multiple of
> OFP_ACTION_ALIGN.

OK, done.

> I would also mention in the description the same warning as in the
> commit message that this may go away in the future.

OK, done.

> > --- a/lib/ofp-parse.c
> > +++ b/lib/ofp-parse.c
> > @@ -282,6 +282,22 @@ str_to_action(char *str, struct ofpbuf *b)
> >             nah = put_action(b, sizeof *nah, OFPAT_VENDOR);
> >             nah->vendor = htonl(NX_VENDOR_ID);
> >             nah->subtype = htons(NXAST_POP_QUEUE);
> > +        } else if (!strcasecmp(act, "note")) {
> > +            struct nx_action_note *nan;
> > +            size_t arg_len, length;
> > +
> > +            if (!arg) {
> > +                arg = "";
> > +            }
> > +            arg_len = strlen(arg);
> > +
> > +            length = ROUND_UP(offsetof(struct nx_action_note, note) + arg_len,
> > +                              OFP_ACTION_ALIGN);
> > +
> > +            nan = put_action(b, length, OFPAT_VENDOR);
> > +            nan->vendor = htonl(NX_VENDOR_ID);
> > +            nan->subtype = htons(NXAST_NOTE);
> > +            memcpy(nan->note, arg, arg_len);
> 
> Looking through the code that translates a string into actions, I
> don't see anything that enforces that a packet will fit in an OpenFlow
> message (at least on the "ovs-ofctl add-flow" code path).  This is a
> problem with normal flows, but it will be exacerbated by not enforcing
> any sort of limits on the length of these notes.

I think that this problem is not really specific to this patch (although
I agree that it makes it more likely to occur), so I've written it down
on my whiteboard as something to look at later.

> While my secret (or not so secret) hope is that we come up with an
> alternate way to store this information, it may be worth documenting
> this capability in the ovs-ofctl man page.

OK, done.

> > static void
> > ofp_print_nx_action(struct ds *string, const struct nx_action_header *nah)
> > {
> > @@ -217,6 +265,10 @@ ofp_print_nx_action(struct ds *string, const struct nx_action_header *nah)
> >         ds_put_cstr(string, "pop_queue");
> >         break;
> > 
> > +    case NXAST_NOTE:
> > +        print_note(string, (const struct nx_action_note *) nah);
> > +        break;
> > +
> 
> I had some concerns about consistency of printing notes with ASCII and
> binary data, which we discussed in person.  Based on that discussion,
> you were going to rework some of this patch so that it always took in
> hex data and output it.

Done.

> > --- a/tests/ovs-ofctl.at
> > +++ b/tests/ovs-ofctl.at
> > @@ -10,6 +10,7 @@ udp dl_vlan_pcp=7 idle_timeout=5 actions=strip_vlan output:0
> > 
> > +actions=note:abc,note:abcdefghi,note:
> > ...
> > +flow_mod: ADD: actions=note:"abc",note:"abcdefghi",note: 7f 00 00 00 00 00
> 
> This backspace wasn't printable in my mail viewer.  If you were to
> keep this test, it would be worth adding a comment to the action
> definition so that people know that you were using a special sequence.

I changed all the tests to use only hex.

Here's the new version.

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Fri, 12 Nov 2010 16:23:26 -0800
Subject: [PATCH] nicira-ext: New Nicira vendor action NXAST_NOTE.

Our controller group at Nicira has requested a way to annotate flows with
extra information beyond the flow cookie.  The new NXAST_NOTE action
provides such a way.

This new action is somewhat controversial.  Some have suggested that it
should be added another way (either as part of the Nicira Extended Match
or as a new component of the flow_mod and related messages).  Others think
that it has no place in the OpenFlow protocol at all and that an equivalent
should be implemented using the already available features of OVSDB.  So
it is possible that this extension will be deleted and the feature will
be reimplemented some other way (or not at all).

CC: Teemu Koponen <koponen at nicira.com>
CC: Jeremy Stribling <strib at nicira.com>
---
 include/openflow/nicira-ext.h |   19 +++++++++++++++++++
 lib/ofp-parse.c               |   40 ++++++++++++++++++++++++++++++++++++++++
 lib/ofp-print.c               |   20 ++++++++++++++++++++
 lib/ofp-util.c                |    6 ++++++
 ofproto/ofproto.c             |    3 +++
 tests/ovs-ofctl.at            |    2 ++
 utilities/ovs-ofctl.8.in      |    4 ++++
 7 files changed, 94 insertions(+), 0 deletions(-)

diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 3c2856f..d87915e 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -230,6 +230,7 @@ enum nx_action_subtype {
     NXAST_POP_QUEUE,            /* struct nx_action_pop_queue */
     NXAST_REG_MOVE,             /* struct nx_action_reg_move */
     NXAST_REG_LOAD,             /* struct nx_action_reg_load */
+    NXAST_NOTE                  /* struct nx_action_note */
 };
 
 /* Header for Nicira-defined actions. */
@@ -442,6 +443,24 @@ struct nx_action_reg_load {
 };
 OFP_ASSERT(sizeof(struct nx_action_reg_load) == 24);
 
+/* Action structure for NXAST_NOTE.
+ *
+ * This action has no effect.  It is variable length.  The switch does not
+ * attempt to interpret the user-defined 'note' data in any way.  A controller
+ * can use this action to attach arbitrary metadata to a flow.
+ *
+ * This action might go away in the future.
+ */
+struct nx_action_note {
+    uint16_t type;                  /* OFPAT_VENDOR. */
+    uint16_t len;                   /* A multiple of 8, but at least 16. */
+    uint32_t vendor;                /* NX_VENDOR_ID. */
+    uint16_t subtype;               /* NXAST_NOTE. */
+    uint8_t note[6];                /* Start of user-defined data. */
+    /* Possibly followed by additional user-defined data. */
+};
+OFP_ASSERT(sizeof(struct nx_action_note) == 16);
+
 /* Wildcard for tunnel ID. */
 #define NXFW_TUN_ID  (1 << 25)
 
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index a9bc0c2..6b53f26 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -277,6 +277,46 @@ str_to_action(char *str, struct ofpbuf *b)
             nah = put_action(b, sizeof *nah, OFPAT_VENDOR);
             nah->vendor = htonl(NX_VENDOR_ID);
             nah->subtype = htons(NXAST_POP_QUEUE);
+        } else if (!strcasecmp(act, "note")) {
+            size_t start_ofs = b->size;
+            struct nx_action_note *nan;
+            int remainder;
+            size_t len;
+
+            nan = put_action(b, sizeof *nan, OFPAT_VENDOR);
+            nan->vendor = htonl(NX_VENDOR_ID);
+            nan->subtype = htons(NXAST_NOTE);
+
+            b->size -= sizeof nan->note;
+            while (arg && *arg != '\0') {
+                int high, low;
+                uint8_t byte;
+
+                if (*arg == '.') {
+                    arg++;
+                }
+                if (*arg == '\0') {
+                    break;
+                }
+
+                high = hexit_value(*arg++);
+                if (high >= 0) {
+                    low = hexit_value(*arg++);
+                }
+                if (high < 0 || low < 0) {
+                    ovs_fatal(0, "bad hex digit in `note' argument");
+                }
+
+                byte = high * 16 + low;
+                ofpbuf_put(b, &byte, 1);
+            }
+
+            len = b->size - start_ofs;
+            remainder = len % OFP_ACTION_ALIGN;
+            if (remainder) {
+                ofpbuf_put_zeros(b, OFP_ACTION_ALIGN - remainder);
+            }
+            nan->len = htons(b->size - start_ofs);
         } else if (!strcasecmp(act, "output")) {
             put_output_action(b, str_to_u32(arg));
         } else if (!strcasecmp(act, "enqueue")) {
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 87ae185..4572db4 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -185,6 +185,22 @@ static void ofp_print_port_name(struct ds *string, uint16_t port)
 }
 
 static void
+print_note(struct ds *string, const struct nx_action_note *nan)
+{
+    size_t len;
+    size_t i;
+
+    ds_put_cstr(string, "note:");
+    len = ntohs(nan->len) - offsetof(struct nx_action_note, note);
+    for (i = 0; i < len; i++) {
+        if (i) {
+            ds_put_char(string, '.');
+        }
+        ds_put_format(string, "%02"PRIx8, nan->note[i]);
+    }
+}
+
+static void
 ofp_print_nx_action(struct ds *string, const struct nx_action_header *nah)
 {
     switch (ntohs(nah->subtype)) {
@@ -217,6 +233,10 @@ ofp_print_nx_action(struct ds *string, const struct nx_action_header *nah)
         ds_put_cstr(string, "pop_queue");
         break;
 
+    case NXAST_NOTE:
+        print_note(string, (const struct nx_action_note *) nah);
+        break;
+
     default:
         ds_put_format(string, "***unknown Nicira action:%d***",
                       ntohs(nah->subtype));
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 4d632ef..d7bc0ee 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -545,6 +545,7 @@ check_nicira_action(const union ofp_action *a, unsigned int len,
     case NXAST_SET_QUEUE:
     case NXAST_POP_QUEUE:
         return check_action_exact_len(a, len, 16);
+
     case NXAST_REG_MOVE:
         error = check_action_exact_len(a, len,
                                        sizeof(struct nx_action_reg_move));
@@ -552,6 +553,7 @@ check_nicira_action(const union ofp_action *a, unsigned int len,
             return error;
         }
         return nxm_check_reg_move((const struct nx_action_reg_move *) a, flow);
+
     case NXAST_REG_LOAD:
         error = check_action_exact_len(a, len,
                                        sizeof(struct nx_action_reg_load));
@@ -559,6 +561,10 @@ check_nicira_action(const union ofp_action *a, unsigned int len,
             return error;
         }
         return nxm_check_reg_load((const struct nx_action_reg_load *) a, flow);
+
+    case NXAST_NOTE:
+        return 0;
+
     default:
         return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_VENDOR_TYPE);
     }
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 61ef581..e84c6a4 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2843,6 +2843,9 @@ xlate_nicira_action(struct action_xlate_ctx *ctx,
     case NXAST_REG_LOAD:
         nxm_execute_reg_load((const struct nx_action_reg_load *) nah,
                              &ctx->flow);
+
+    case NXAST_NOTE:
+        /* Nothing to do. */
         break;
 
     /* If you add a new action here that modifies flow data, don't forget to
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index de73e57..88d50a5 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -10,6 +10,7 @@ udp dl_vlan_pcp=7 idle_timeout=5 actions=strip_vlan output:0
 tcp,nw_src=192.168.0.3,tp_dst=80 actions=set_queue:37,output:1
 udp,nw_src=192.168.0.3,tp_dst=53 actions=pop_queue,output:1
 cookie=0x123456789abcdef hard_timeout=10 priority=60000 actions=controller
+actions=note:41.42.43,note:00.01.02.03.04.05.06.07,note
 actions=drop
 ])
 AT_CHECK([ovs-ofctl parse-flows flows.txt], [0], [stdout])
@@ -21,6 +22,7 @@ flow_mod: udp,dl_vlan_pcp=7, ADD: idle:5 actions=strip_vlan,output:0
 flow_mod: tcp,nw_src=192.168.0.3,tp_dst=80, ADD: actions=set_queue:37,output:1
 flow_mod: udp,nw_src=192.168.0.3,tp_dst=53, ADD: actions=pop_queue,output:1
 flow_mod: ADD: cookie:0x123456789abcdef hard:10 pri:60000 actions=CONTROLLER:65535
+flow_mod: ADD: actions=note:41.42.43.00.00.00,note:00.01.02.03.04.05.06.07.00.00.00.00.00.00,note:00.00.00.00.00.00
 flow_mod: ADD: actions=drop
 ])
 AT_CLEANUP
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 42c8ab5..7ffad2b 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -479,6 +479,10 @@ OpenFlow implementations do not support queuing at all.
 Restores the queue to the value it was before any \fBset_queue\fR
 actions were applied.
 .
+.IP \fBnote:\fR[\fIhh\fR]...
+Does nothing at all.  Any number of bytes represented as hex digits
+\fIhh\fR may be included.  Pairs of hex digits may be separated by
+periods for readability.
 .RE
 .
 .IP
-- 
1.7.1





More information about the dev mailing list