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 <[email protected]> Acked-by: Simon Horman <[email protected]>
huikang · Nov 4, 2015 · 68dfc25 · 68dfc25
1 parent 337c452
commit 68dfc25
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 35 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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