[ovs-dev] [PATCH 2/2] ofproto: correct group fields command line option parsing

Simon Horman simon.horman at netronome.com
Fri Oct 16 10:50:48 UTC 2015


This corrects the parsing of 'fields' specified for groups on
the command line. 'fields' may be used in conjunction with the
Netronome selection method extension to describe which fields of
the flow should be used as by the selection method.

This patch corrects two problems with the current implementation
as compared to the documentation in the ovs-ofctl man page.
* Allows parsing of more than one field
* Allows parsing of masks for fields

Fixes: 18ac06d3546e ("ofp-util: Encoding and decoding of (draft) OpenFlow 1.5 group messages.")
Signed-off-by: Simon Horman <simon.horman at netronome.com>
---
 lib/ofp-parse.c  | 112 ++++++++++++++++++++++++++++++-------------------------
 tests/ofproto.at |   6 +--
 2 files changed, 65 insertions(+), 53 deletions(-)

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 843765650a44..647e019db35d 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1225,62 +1225,54 @@ parse_bucket_str(struct ofputil_bucket *bucket, char *str_, uint8_t group_type,
 }
 
 static char * OVS_WARN_UNUSED_RESULT
-parse_select_group_field(char *s, struct field_array *fa,
+parse_select_group_field(const char *name, const char *value_str,
+                         struct field_array *fa,
                          enum ofputil_protocol *usable_protocols)
 {
-    char *save_ptr = NULL;
-    char *name;
+    const struct mf_field *mf = mf_from_name(name);
 
-    for (name = strtok_r(s, "=, \t\r\n", &save_ptr); name;
-         name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
-        const struct mf_field *mf = mf_from_name(name);
+    if (mf) {
+        char *error;
+        union mf_value value;
 
-        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);
+        }
 
-            if (bitmap_is_set(fa->used.bm, mf->id)) {
-                return xasprintf("%s: duplicate field", name);
+        if (value_str) {
+            error = mf_parse_value(mf, value_str, &value);
+            if (error) {
+                return error;
             }
 
-            value_str = strtok_r(NULL, ", \t\r\n", &save_ptr);
-            if (value_str) {
-                error = mf_parse_value(mf, value_str, &value);
-                if (error) {
-                    return error;
-                }
-
-                /* The mask cannot be all-zeros */
-                if (!mf_is_tun_metadata(mf) &&
-                    is_all_zeros(&value, mf->n_bytes)) {
-                    return xasprintf("%s: values are wildcards here "
-                                     "and must not be all-zeros", s);
-                }
+            /* The mask cannot be all-zeros */
+            if (!mf_is_tun_metadata(mf) &&
+                is_all_zeros(&value, mf->n_bytes)) {
+                return xasprintf("%s: values are wildcards here "
+                                 "and must not be all-zeros", name);
+            }
 
-                /* The values parsed are masks for fields used
-                 * by the selection method */
-                if (!mf_is_mask_valid(mf, &value)) {
-                    return xasprintf("%s: invalid mask for field %s",
-                                     value_str, mf->name);
-                }
-            } else {
-                memset(&value, 0xff, mf->n_bytes);
+            /* The values parsed are masks for fields used
+             * by the selection method */
+            if (!mf_is_mask_valid(mf, &value)) {
+                return xasprintf("%s: invalid mask for field", name);
             }
+        } else {
+            memset(&value, 0xff, mf->n_bytes);
+        }
 
-            field_array_set(mf->id, &value, fa);
+        field_array_set(mf->id, &value, fa);
 
-            if (is_all_ones(&value, mf->n_bytes)) {
-                *usable_protocols &= mf->usable_protocols_exact;
-            } else if (mf->usable_protocols_bitwise == mf->usable_protocols_cidr
-                       || ip_is_cidr(value.be32)) {
-                *usable_protocols &= mf->usable_protocols_cidr;
-            } else {
-                *usable_protocols &= mf->usable_protocols_bitwise;
-            }
+        if (is_all_ones(&value, mf->n_bytes)) {
+            *usable_protocols &= mf->usable_protocols_exact;
+        } else if (mf->usable_protocols_bitwise == mf->usable_protocols_cidr
+                   || ip_is_cidr(value.be32)) {
+            *usable_protocols &= mf->usable_protocols_cidr;
         } else {
-            return xasprintf("%s: unknown field %s", s, name);
+            *usable_protocols &= mf->usable_protocols_bitwise;
         }
+    } else {
+        return xasprintf("unknown select group field %s", name);
     }
 
     return NULL;
@@ -1300,6 +1292,7 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
     char *save_ptr = NULL;
     bool had_type = false;
     bool had_command_bucket_id = false;
+    bool parsing_fields = false;
     char *name;
     struct ofputil_bucket *bucket;
     char *error = NULL;
@@ -1358,14 +1351,29 @@ 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)) {
+    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 (parsing_fields &&
+            (!strcmp(name, "command_bucket_id") ||
+             !strcmp(name, "group_id") ||
+             !strcmp(name, "type") ||
+             !strcmp(name, "selection_method") ||
+             !strcmp(name, "selection_method") ||
+             !strcmp(name, "selection_method_param"))) {
+             parsing_fields = false;
+        }
+
+again:
+        value = strchr(name, '=');
         if (!value) {
-            error = xasprintf("field %s missing value", name);
-            goto out;
+            if (!parsing_fields) {
+                error = xasprintf("field %s missing value", name);
+                goto out;
+            }
+        } else {
+            *(value)++ = '\0';
         }
 
         if (!strcmp(name, "command_bucket_id")) {
@@ -1462,12 +1470,16 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
                 error = xstrdup("fields are not needed");
                 goto out;
             }
-            error = parse_select_group_field(value, &gm->props.fields,
+            *usable_protocols &= OFPUTIL_P_OF15_UP;
+            parsing_fields = true;
+            name = value;
+            goto again;
+        } else if (parsing_fields) {
+            error = parse_select_group_field(name, value, &gm->props.fields,
                                              usable_protocols);
             if (error) {
                 goto out;
             }
-            *usable_protocols &= OFPUTIL_P_OF15_UP;
         } else {
             error = xasprintf("unknown keyword %s", name);
             goto out;
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 8699c4a29ed9..e0c4b68ec7f9 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -343,7 +343,7 @@ 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=1233,type=select,selection_method=hash,bucket=output:10,bucket=output:11
+group_id=1233,type=select,fields=eth_src,eth_dst=ff:ff:ff:00:00:00,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=1235,type=all,bucket=actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
 ])
@@ -358,14 +358,14 @@ AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5):
  group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
- group_id=1233,type=select,selection_method=hash,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+ group_id=1233,type=select,selection_method=hash,fields=eth_src,eth_dst=ff:ff:ff:00:00:00,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])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_GROUP_DESC reply (OF1.5):
  group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
- group_id=1233,type=select,selection_method=hash,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
+ group_id=1233,type=select,selection_method=hash,fields=eth_src,eth_dst=ff:ff:ff:00:00:00,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11
 ])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0], [0])
 AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])
-- 
2.1.4




More information about the dev mailing list