[ovs-dev,1/3] ofp-parse: Separate fields properly.
diff mbox

Message ID 1441076226-60939-1-git-send-email-jesse@nicira.com
State Accepted
Headers show

Commit Message

Jesse Gross Sept. 1, 2015, 2:57 a.m. UTC
Currently, each token in an OpenFlow match field is treated separately -
whether this is a name, a value, or a single identifier. However, this
means that attempting to get a value may result in grabbing the next
token if no value exists. This avoids that problem by breaking the match
string down into its components and then individually separating it into
name/value pairs if appropriate.

Signed-off-by: Jesse Gross <jesse@nicira.com>
---
 lib/ofp-parse.c | 165 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 81 insertions(+), 84 deletions(-)

Comments

Ben Pfaff Sept. 8, 2015, 8:48 p.m. UTC | #1
On Mon, Aug 31, 2015 at 07:57:04PM -0700, Jesse Gross wrote:
> Currently, each token in an OpenFlow match field is treated separately -
> whether this is a name, a value, or a single identifier. However, this
> means that attempting to get a value may result in grabbing the next
> token if no value exists. This avoids that problem by breaking the match
> string down into its components and then individually separating it into
> name/value pairs if appropriate.
> 
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Did you consider using ofputil_parse_key_value()?  It is, apparently,
tailor-made for this, but I guess some of the details might make it
incompatible with the legacy format that parse_ofp_str__() deals with.

Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross Sept. 9, 2015, 4:50 p.m. UTC | #2
On Tue, Sep 8, 2015 at 1:48 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Mon, Aug 31, 2015 at 07:57:04PM -0700, Jesse Gross wrote:
>> Currently, each token in an OpenFlow match field is treated separately -
>> whether this is a name, a value, or a single identifier. However, this
>> means that attempting to get a value may result in grabbing the next
>> token if no value exists. This avoids that problem by breaking the match
>> string down into its components and then individually separating it into
>> name/value pairs if appropriate.
>>
>> Signed-off-by: Jesse Gross <jesse@nicira.com>
>
> Did you consider using ofputil_parse_key_value()?  It is, apparently,
> tailor-made for this, but I guess some of the details might make it
> incompatible with the legacy format that parse_ofp_str__() deals with.

It's a good idea - it ends up being almost a drop in replacement for
the current code. I made that change, which also makes this patch much
smaller.

Patch
diff mbox

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index eaaa8ba..a6190ed 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -256,7 +256,7 @@  parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
     } fields;
     char *save_ptr = NULL;
     char *act_str = NULL;
-    char *name;
+    char *field;
 
     *usable_protocols = OFPUTIL_P_ANY;
 
@@ -339,116 +339,113 @@  parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
             return xstrdup("must specify an action");
         }
     }
-    for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name;
-         name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
+    for (field = strtok_r(string, ", \t\r\n", &save_ptr); field;
+         field = strtok_r(NULL, ", \t\r\n", &save_ptr)) {
         const struct protocol *p;
         char *error = NULL;
 
-        if (parse_protocol(name, &p)) {
+        if (parse_protocol(field, &p)) {
             match_set_dl_type(&fm->match, htons(p->dl_type));
             if (p->nw_proto) {
                 match_set_nw_proto(&fm->match, p->nw_proto);
             }
-        } else if (fields & F_FLAGS && !strcmp(name, "send_flow_rem")) {
+        } else if (fields & F_FLAGS && !strcmp(field, "send_flow_rem")) {
             fm->flags |= OFPUTIL_FF_SEND_FLOW_REM;
-        } else if (fields & F_FLAGS && !strcmp(name, "check_overlap")) {
+        } else if (fields & F_FLAGS && !strcmp(field, "check_overlap")) {
             fm->flags |= OFPUTIL_FF_CHECK_OVERLAP;
-        } else if (fields & F_FLAGS && !strcmp(name, "reset_counts")) {
+        } else if (fields & F_FLAGS && !strcmp(field, "reset_counts")) {
             fm->flags |= OFPUTIL_FF_RESET_COUNTS;
             *usable_protocols &= OFPUTIL_P_OF12_UP;
-        } else if (fields & F_FLAGS && !strcmp(name, "no_packet_counts")) {
+        } else if (fields & F_FLAGS && !strcmp(field, "no_packet_counts")) {
             fm->flags |= OFPUTIL_FF_NO_PKT_COUNTS;
             *usable_protocols &= OFPUTIL_P_OF13_UP;
-        } else if (fields & F_FLAGS && !strcmp(name, "no_byte_counts")) {
+        } else if (fields & F_FLAGS && !strcmp(field, "no_byte_counts")) {
             fm->flags |= OFPUTIL_FF_NO_BYT_COUNTS;
             *usable_protocols &= OFPUTIL_P_OF13_UP;
-        } else if (!strcmp(name, "no_readonly_table")
-                   || !strcmp(name, "allow_hidden_fields")) {
+        } else if (!strcmp(field, "no_readonly_table")
+                   || !strcmp(field, "allow_hidden_fields")) {
              /* ignore these fields. */
-        } else if (mf_from_name(name)) {
-            char *value;
-
-            value = strtok_r(NULL, ", \t\r\n", &save_ptr);
-            if (!value) {
-                /* If there's no value, we're just trying to match on the
-                 * existence of the field, so use a no-op value. */
-                value = "0/0";
-            }
-
-            error = parse_field(mf_from_name(name), value, &fm->match,
-                                usable_protocols);
-            if (error) {
-                return error;
-            }
         } else {
-            char *value;
+            char *name, *value;
 
-            value = strtok_r(NULL, ", \t\r\n", &save_ptr);
-            if (!value) {
-                return xasprintf("field %s missing value", name);
-            }
+            name = strsep(&field, "=");
+            value = field;
 
-            if (!strcmp(name, "table")) {
-                error = str_to_u8(value, "table", &fm->table_id);
-                if (fm->table_id != 0xff) {
-                    *usable_protocols &= OFPUTIL_P_TID;
+            if (mf_from_name(name)) {
+                if (!value) {
+                    /* If there's no value, we're just trying to match on the
+                     * existence of the field, so use a no-op value. */
+                     value = "0/0";
                 }
-            } else if (fields & F_OUT_PORT && !strcmp(name, "out_port")) {
-                if (!ofputil_port_from_string(value, &fm->out_port)) {
-                    error = xasprintf("%s is not a valid OpenFlow port",
-                                      value);
+                error = parse_field(mf_from_name(name), value, &fm->match,
+                                    usable_protocols);
+            } else {
+                if (!value) {
+                    return xasprintf("field %s missing value", name);
                 }
-            } else if (fields & F_PRIORITY && !strcmp(name, "priority")) {
-                uint16_t priority = 0;
-
-                error = str_to_u16(value, name, &priority);
-                fm->priority = priority;
-            } else if (fields & F_TIMEOUT && !strcmp(name, "idle_timeout")) {
-                error = str_to_u16(value, name, &fm->idle_timeout);
-            } else if (fields & F_TIMEOUT && !strcmp(name, "hard_timeout")) {
-                error = str_to_u16(value, name, &fm->hard_timeout);
-            } else if (fields & F_IMPORTANCE && !strcmp(name, "importance")) {
-                error = str_to_u16(value, name, &fm->importance);
-            } else if (!strcmp(name, "cookie")) {
-                char *mask = strchr(value, '/');
 
-                if (mask) {
-                    /* A mask means we're searching for a cookie. */
-                    if (command == OFPFC_ADD) {
-                        return xstrdup("flow additions cannot use "
-                                       "a cookie mask");
+                if (!strcmp(name, "table")) {
+                    error = str_to_u8(value, "table", &fm->table_id);
+                    if (fm->table_id != 0xff) {
+                        *usable_protocols &= OFPUTIL_P_TID;
                     }
-                    *mask = '\0';
-                    error = str_to_be64(value, &fm->cookie);
-                    if (error) {
-                        return error;
+                } else if (fields & F_OUT_PORT && !strcmp(name, "out_port")) {
+                    if (!ofputil_port_from_string(value, &fm->out_port)) {
+                        error = xasprintf("%s is not a valid OpenFlow port",
+                                          value);
                     }
-                    error = str_to_be64(mask + 1, &fm->cookie_mask);
-
-                    /* Matching of the cookie is only supported through NXM or
-                     * OF1.1+. */
-                    if (fm->cookie_mask != htonll(0)) {
-                        *usable_protocols &= OFPUTIL_P_NXM_OF11_UP;
+                } else if (fields & F_PRIORITY && !strcmp(name, "priority")) {
+                    uint16_t priority = 0;
+
+                    error = str_to_u16(value, name, &priority);
+                    fm->priority = priority;
+                } else if (fields & F_TIMEOUT && !strcmp(name, "idle_timeout")) {
+                    error = str_to_u16(value, name, &fm->idle_timeout);
+                } else if (fields & F_TIMEOUT && !strcmp(name, "hard_timeout")) {
+                    error = str_to_u16(value, name, &fm->hard_timeout);
+                } else if (fields & F_IMPORTANCE && !strcmp(name, "importance")) {
+                    error = str_to_u16(value, name, &fm->importance);
+                } else if (!strcmp(name, "cookie")) {
+                    char *mask = strchr(value, '/');
+
+                    if (mask) {
+                        /* A mask means we're searching for a cookie. */
+                        if (command == OFPFC_ADD) {
+                            return xstrdup("flow additions cannot use "
+                                           "a cookie mask");
+                        }
+                        *mask = '\0';
+                        error = str_to_be64(value, &fm->cookie);
+                        if (error) {
+                            return error;
+                        }
+                        error = str_to_be64(mask + 1, &fm->cookie_mask);
+
+                        /* Matching of the cookie is only supported through
+                         * NXM or OF1.1+. */
+                        if (fm->cookie_mask != htonll(0)) {
+                            *usable_protocols &= OFPUTIL_P_NXM_OF11_UP;
+                        }
+                    } else {
+                        /* No mask means that the cookie is being set. */
+                        if (command != OFPFC_ADD && command != OFPFC_MODIFY
+                            && command != OFPFC_MODIFY_STRICT) {
+                            return xstrdup("cannot set cookie");
+                        }
+                        error = str_to_be64(value, &fm->new_cookie);
+                        fm->modify_cookie = true;
                     }
+                } else if (!strcmp(name, "duration")
+                           || !strcmp(name, "n_packets")
+                           || !strcmp(name, "n_bytes")
+                           || !strcmp(name, "idle_age")
+                           || !strcmp(name, "hard_age")) {
+                    /* Ignore these, so that users can feed the output of
+                     * "ovs-ofctl dump-flows" back into commands that parse
+                     * flows. */
                 } else {
-                    /* No mask means that the cookie is being set. */
-                    if (command != OFPFC_ADD && command != OFPFC_MODIFY
-                        && command != OFPFC_MODIFY_STRICT) {
-                        return xstrdup("cannot set cookie");
-                    }
-                    error = str_to_be64(value, &fm->new_cookie);
-                    fm->modify_cookie = true;
+                    error = xasprintf("unknown keyword %s", name);
                 }
-            } else if (!strcmp(name, "duration")
-                       || !strcmp(name, "n_packets")
-                       || !strcmp(name, "n_bytes")
-                       || !strcmp(name, "idle_age")
-                       || !strcmp(name, "hard_age")) {
-                /* Ignore these, so that users can feed the output of
-                 * "ovs-ofctl dump-flows" back into commands that parse
-                 * flows. */
-            } else {
-                error = xasprintf("unknown keyword %s", name);
             }
 
             if (error) {