diff mbox

[ovs-dev] ofp-parse: Fix parsing, formatting of multiple fields in NTR extension.

Message ID 1445008866-5572-1-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Oct. 16, 2015, 3:21 p.m. UTC 
Until now, the only way to specify multiple fields in the "fields"
parameter for the Netronome groups extension, was to specify "fields"
more than once, e.g. fields=eth_dst,fields=ip_dst

However, this wasn't documented and the code in ofp-print didn't use it,
generating output that couldn't be parsed.

This commit fixes the situation by introducing a more straightforward
syntax, e.g. fields(eth_dst,ip_dst), documents it, and adjusts ofp-print
code to use it when there is more than one field (it retains the previous
format for backward compatibility when there is exactly one field)

CC: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 lib/ofp-parse.c          | 25 ++++++-------------------
 lib/ofp-print.c          | 22 ++++++++++------------
 tests/ofp-print.at       |  2 +-
 tests/ofproto.at         |  4 ++--
 utilities/ovs-ofctl.8.in |  6 +++++-
 5 files changed, 24 insertions(+), 35 deletions(-)

Comments

Simon Horman Oct. 19, 2015, 2:17 a.m. UTC | #1 
On Fri, Oct 16, 2015 at 08:21:06AM -0700, Ben Pfaff wrote:
> Until now, the only way to specify multiple fields in the "fields"
> parameter for the Netronome groups extension, was to specify "fields"
> more than once, e.g. fields=eth_dst,fields=ip_dst
> 
> However, this wasn't documented and the code in ofp-print didn't use it,
> generating output that couldn't be parsed.
> 
> This commit fixes the situation by introducing a more straightforward
> syntax, e.g. fields(eth_dst,ip_dst), documents it, and adjusts ofp-print
> code to use it when there is more than one field (it retains the previous
> format for backward compatibility when there is exactly one field)
> 
> CC: Simon Horman <simon.horman@netronome.com>
> Signed-off-by: Ben Pfaff <blp@nicira.com>

Acked-by: Simon Horman <simon.horman@netronome.com>
Ben Pfaff Nov. 4, 2015, 3:53 a.m. UTC | #2 
On Mon, Oct 19, 2015 at 11:17:16AM +0900, Simon Horman wrote:
> On Fri, Oct 16, 2015 at 08:21:06AM -0700, Ben Pfaff wrote:
> > Until now, the only way to specify multiple fields in the "fields"
> > parameter for the Netronome groups extension, was to specify "fields"
> > more than once, e.g. fields=eth_dst,fields=ip_dst
> > 
> > However, this wasn't documented and the code in ofp-print didn't use it,
> > generating output that couldn't be parsed.
> > 
> > This commit fixes the situation by introducing a more straightforward
> > syntax, e.g. fields(eth_dst,ip_dst), documents it, and adjusts ofp-print
> > code to use it when there is more than one field (it retains the previous
> > format for backward compatibility when there is exactly one field)
> > 
> > CC: Simon Horman <simon.horman@netronome.com>
> > Signed-off-by: Ben Pfaff <blp@nicira.com>
> 
> Acked-by: Simon Horman <simon.horman@netronome.com>

Thanks, applied to master and branch-2.4.
Simon Horman Nov. 4, 2015, 7:04 a.m. UTC | #3 
On Tue, Nov 03, 2015 at 07:53:06PM -0800, Ben Pfaff wrote:
> On Mon, Oct 19, 2015 at 11:17:16AM +0900, Simon Horman wrote:
> > On Fri, Oct 16, 2015 at 08:21:06AM -0700, Ben Pfaff wrote:
> > > Until now, the only way to specify multiple fields in the "fields"
> > > parameter for the Netronome groups extension, was to specify "fields"
> > > more than once, e.g. fields=eth_dst,fields=ip_dst
> > > 
> > > However, this wasn't documented and the code in ofp-print didn't use it,
> > > generating output that couldn't be parsed.
> > > 
> > > This commit fixes the situation by introducing a more straightforward
> > > syntax, e.g. fields(eth_dst,ip_dst), documents it, and adjusts ofp-print
> > > code to use it when there is more than one field (it retains the previous
> > > format for backward compatibility when there is exactly one field)
> > > 
> > > CC: Simon Horman <simon.horman@netronome.com>
> > > Signed-off-by: Ben Pfaff <blp@nicira.com>
> > 
> > Acked-by: Simon Horman <simon.horman@netronome.com>
> 
> Thanks, applied to master and branch-2.4.

Great, thanks.
diff mbox

Patch

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 8437656..214cc36 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1228,24 +1228,20 @@  static char * OVS_WARN_UNUSED_RESULT
 parse_select_group_field(char *s, struct field_array *fa,
                          enum ofputil_protocol *usable_protocols)
 {
-    char *save_ptr = NULL;
-    char *name;
+    char *name, *value_str;
 
-    for (name = strtok_r(s, "=, \t\r\n", &save_ptr); name;
-         name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
+    while (ofputil_parse_key_value(&s, &name, &value_str)) {
         const struct mf_field *mf = mf_from_name(name);
 
         if (mf) {
             char *error;
-            const char *value_str;
             union mf_value value;
 
             if (bitmap_is_set(fa->used.bm, mf->id)) {
                 return xasprintf("%s: duplicate field", name);
             }
 
-            value_str = strtok_r(NULL, ", \t\r\n", &save_ptr);
-            if (value_str) {
+            if (*value_str) {
                 error = mf_parse_value(mf, value_str, &value);
                 if (error) {
                     return error;
@@ -1297,10 +1293,8 @@  parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
         F_COMMAND_BUCKET_ID     = 1 << 2,
         F_COMMAND_BUCKET_ID_ALL = 1 << 3,
     } fields;
-    char *save_ptr = NULL;
     bool had_type = false;
     bool had_command_bucket_id = false;
-    char *name;
     struct ofputil_bucket *bucket;
     char *error = NULL;
 
@@ -1358,16 +1352,9 @@  parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
     }
 
     /* Parse everything before the buckets. */
-    for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name;
-         name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
-        char *value;
-
-        value = strtok_r(NULL, ", \t\r\n", &save_ptr);
-        if (!value) {
-            error = xasprintf("field %s missing value", name);
-            goto out;
-        }
-
+    char *pos = string;
+    char *name, *value;
+    while (ofputil_parse_key_value(&pos, &name, &value)) {
         if (!strcmp(name, "command_bucket_id")) {
             if (!(fields & F_COMMAND_BUCKET_ID)) {
                 error = xstrdup("command bucket id is not needed");
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index d0c94ce..240ba84 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2352,22 +2352,20 @@  ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type,
     }
 
     if (props->selection_method[0]) {
-        size_t mark, start;
-
-        ds_put_format(s, ",selection_method=%s,", props->selection_method);
+        ds_put_format(s, ",selection_method=%s", props->selection_method);
         if (props->selection_method_param) {
-            ds_put_format(s, "selection_method_param=%"PRIu64",",
+            ds_put_format(s, ",selection_method_param=%"PRIu64,
                           props->selection_method_param);
         }
 
-        /* Allow rewinding to immediately before the trailing ',' */
-        mark = s->length - 1;
-
-        ds_put_cstr(s, "fields=");
-        start = s->length;
-        oxm_format_field_array(s, &props->fields);
-        if (s->length == start) {
-            ds_truncate(s, mark);
+        size_t n = bitmap_count1(props->fields.used.bm, MFF_N_IDS);
+        if (n == 1) {
+            ds_put_cstr(s, ",fields=");
+            oxm_format_field_array(s, &props->fields);
+        } else if (n > 1) {
+            ds_put_cstr(s, ",fields(");
+            oxm_format_field_array(s, &props->fields);
+            ds_put_char(s, ')');
         }
     }
 
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index 8cdcead..fd9c2d4 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -2043,7 +2043,7 @@  ff ff 00 3b 00 00 15 40 00 00 00 01 00 00 00 00 \
 14 01 ff 00 00 00 00 00 \
 "], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5) (xid=0x2):
- group_id=8192,type=select,selection_method=hash,fields=ip_dst=255.255.255.0,nw_proto,tcp_src,bucket=bucket_id:0,weight:100,watch_port:1,actions=output:1,bucket=bucket_id:1,weight:200,watch_port:2,actions=output:2,bucket=bucket_id:2,weight:200,watch_port:3,actions=output:3
+ group_id=8192,type=select,selection_method=hash,fields(ip_dst=255.255.255.0,nw_proto,tcp_src),bucket=bucket_id:0,weight:100,watch_port:1,actions=output:1,bucket=bucket_id:1,weight:200,watch_port:2,actions=output:2,bucket=bucket_id:2,weight:200,watch_port:3,actions=output:3
 ])
 AT_CLEANUP
 
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 5e4441c..beeb65a 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -343,14 +343,14 @@  dnl Actions definition listed in both supported formats (w/ actions=)
 AT_SETUP([ofproto - del group (OpenFlow 1.5)])
 OVS_VSWITCHD_START
 AT_DATA([groups.txt], [dnl
-group_id=1234,type=select,selection_method=hash,bucket=output:10,bucket=output:11
+group_id=1234,type=select,selection_method=hash,fields(eth_dst,ip_dst,tcp_dst),bucket=output:10,bucket=output:11
 group_id=1235,type=all,bucket=actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn add-groups br0 groups.txt])
 AT_CHECK([ovs-ofctl -F OXM-OpenFlow15 -O OpenFlow15 -vwarn dump-groups br0 1234], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5):
- group_id=1234,type=select,selection_method=hash,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+ group_id=1234,type=select,selection_method=hash,fields(eth_dst,ip_dst,tcp_dst),bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0 group_id=1234])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index a6087f6..dc81e7e 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -2596,6 +2596,9 @@  This field is optional for \fBadd\-group\fR, \fBadd\-groups\fR and
 \fBmod\-group\fR commands on groups of type \fBselect\fR. Prohibited
 otherwise. The default value is the empty string.
 .IP
+Other than the empty string, \fBhash\fR is currently the only defined
+selection method.
+.IP
 This option will use a Netronome OpenFlow extension which is only supported
 when using Open vSwitch 2.4 and later with OpenFlow 1.5 and later.
 
@@ -2609,7 +2612,8 @@  Prohibited otherwise. The default value is zero.
 This option will use a Netronome OpenFlow extension which is only supported
 when using Open vSwitch 2.4 and later with OpenFlow 1.5 and later.
 
-.IP \fBfields\fR=\fIparam\fR
+.IP \fBfields\fR=\fIfield\fR
+.IQ \fBfields(\fIfield\fR[\fB=\fImask\fR]\fR...\fB)\fR
 The field parameters to selection method selected by the
 \fBselection_method\fR field.  The syntax is described in \fBFlow Syntax\fR
 with the additional restrictions that if a value is provided it is