ofp-parse: Fix parsing, formatting of multiple fields in NTR extension.

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)

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Simon Horman <simon.horman@netronome.com>

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 8437656..214cc36 100644 (file)
--- 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 (file)
--- 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 25cf950..fce7671 100644 (file)
--- 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 732c1a9..634735b 100644 (file)
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -344,14 +344,14 @@ AT_SETUP([ofproto - del group (OpenFlow 1.5)])
 OVS_VSWITCHD_START
 AT_DATA([groups.txt], [dnl
 group_id=1233,type=select,selection_method=hash,bucket=output:10,bucket=output:11
-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 (file)
--- 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