diff mbox

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

Message ID 1444992648-27107-3-git-send-email-simon.horman@netronome.com
State Not Applicable
Headers show

Commit Message

Simon Horman Oct. 16, 2015, 10:50 a.m. UTC 
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@netronome.com>
---
 lib/ofp-parse.c  | 112 ++++++++++++++++++++++++++++++-------------------------
 tests/ofproto.at |   6 +--
 2 files changed, 65 insertions(+), 53 deletions(-)

Comments

Ben Pfaff Oct. 16, 2015, 3:22 p.m. UTC | #1 
On Fri, Oct 16, 2015 at 07:50:48PM +0900, Simon Horman wrote:
> 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@netronome.com>

I wrote a patch yesterday that did the same thing, but now I see that I
failed to post it.  I've posted it now.  Can you take a look at it and
compare the effects?
        http://openvswitch.org/pipermail/dev/2015-October/061318.html
Simon Horman Oct. 19, 2015, 2:18 a.m. UTC | #2 
On Fri, Oct 16, 2015 at 08:22:06AM -0700, Ben Pfaff wrote:
> On Fri, Oct 16, 2015 at 07:50:48PM +0900, Simon Horman wrote:
> > 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@netronome.com>
> 
> I wrote a patch yesterday that did the same thing, but now I see that I
> failed to post it.  I've posted it now.  Can you take a look at it and
> compare the effects?
>         http://openvswitch.org/pipermail/dev/2015-October/061318.html

Funny that we were both looking at that problem at about the same time.
Your approach looks good to me and I have Acked it accordingly.
diff mbox

Patch

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])