Fix treatment of OpenFlow 1.1+ bucket weights.

Until now, OVS has parsed all OF1.1+ group buckets that lack a weight
as having weight 1.  Unfortunately, OpenFlow says that only "select"
groups may have a nonzero weight, and requires reporting an error for
other kinds of groups that have a nonzero weight.  This commit fixes
the problem by parsing only select groups with a default weight of 1
and other groups with a default weight of 0.  It also adds the
OpenFlow-required check for nonzero weights for other kinds of groups.

This complies with OpenFlow 1.1 and later.  OF1.1 says in section 5.8:

    If a specified group type is invalid (ie: includes fields such as
    weight that are undefined for the specified group type) then the
    switch must refuse to add the group entry and must send an
    ofp_error_msg with OFPET_GROUP_MOD_FAILED type and
    OFPGMFC_INVALID_GROUP code.

Found by OFTest.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Flavio Leitner <fbl@sysclose.org>

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 210feed..43b2100 100644 (file)
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1123,7 +1123,7 @@ exit:
 }
 
 static char * OVS_WARN_UNUSED_RESULT
-parse_bucket_str(struct ofputil_bucket *bucket, char *str_,
+parse_bucket_str(struct ofputil_bucket *bucket, char *str_, uint8_t group_type,
                   enum ofputil_protocol *usable_protocols)
 {
     char *pos, *key, *value;
@@ -1131,7 +1131,7 @@ parse_bucket_str(struct ofputil_bucket *bucket, char *str_,
     struct ds actions;
     char *error;
 
-    bucket->weight = 1;
+    bucket->weight = group_type == OFPGT11_SELECT ? 1 : 0;
     bucket->bucket_id = OFPG15_BUCKET_ALL;
     bucket->watch_port = OFPP_ANY;
     bucket->watch_group = OFPG11_ANY;
@@ -1313,40 +1313,19 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
 
     *usable_protocols = OFPUTIL_P_OF11_UP;
 
-    if (fields & F_BUCKETS) {
-        char *bkt_str = strstr(string, "bucket=");
-
-        if (bkt_str) {
-            *bkt_str = '\0';
-        }
-
-        while (bkt_str) {
-            char *next_bkt_str;
-
-            bkt_str = strchr(bkt_str + 1, '=');
-            if (!bkt_str) {
-                error = xstrdup("must specify bucket content");
-                goto out;
-            }
-            bkt_str++;
-
-            next_bkt_str = strstr(bkt_str, "bucket=");
-            if (next_bkt_str) {
-                *next_bkt_str = '\0';
-            }
-
-            bucket = xzalloc(sizeof(struct ofputil_bucket));
-            error = parse_bucket_str(bucket, bkt_str, usable_protocols);
-            if (error) {
-                free(bucket);
-                goto out;
-            }
-            list_push_back(&gm->buckets, &bucket->list_node);
-
-            bkt_str = next_bkt_str;
+    /* Strip the buckets off the end of 'string', if there are any, saving a
+     * pointer for later.  We want to parse the buckets last because the bucket
+     * type influences bucket defaults. */
+    char *bkt_str = strstr(string, "bucket=");
+    if (bkt_str) {
+        if (!(fields & F_BUCKETS)) {
+            error = xstrdup("bucket is not needed");
+            goto out;
         }
+        *bkt_str = '\0';
     }
 
+    /* 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;
@@ -1421,9 +1400,6 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
                 goto out;
             }
             had_type = true;
-        } else if (!strcmp(name, "bucket")) {
-            error = xstrdup("bucket is not needed");
-            goto out;
         } else if (!strcmp(name, "selection_method")) {
             if (!(fields & F_GROUP_TYPE)) {
                 error = xstrdup("selection method is not needed");
@@ -1484,12 +1460,36 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
         goto out;
     }
 
-    /* Validate buckets. */
-    LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
-        if (bucket->weight != 1 && gm->type != OFPGT11_SELECT) {
+    /* Now parse the buckets, if any. */
+    while (bkt_str) {
+        char *next_bkt_str;
+
+        bkt_str = strchr(bkt_str + 1, '=');
+        if (!bkt_str) {
+            error = xstrdup("must specify bucket content");
+            goto out;
+        }
+        bkt_str++;
+
+        next_bkt_str = strstr(bkt_str, "bucket=");
+        if (next_bkt_str) {
+            *next_bkt_str = '\0';
+        }
+
+        bucket = xzalloc(sizeof(struct ofputil_bucket));
+        error = parse_bucket_str(bucket, bkt_str, gm->type, usable_protocols);
+        if (error) {
+            free(bucket);
+            goto out;
+        }
+        list_push_back(&gm->buckets, &bucket->list_node);
+
+        if (gm->type != OFPGT11_SELECT && bucket->weight) {
             error = xstrdup("Only select groups can have bucket weights.");
             goto out;
         }
+
+        bkt_str = next_bkt_str;
     }
     if (gm->type == OFPGT11_INDIRECT && !list_is_short(&gm->buckets)) {
         error = xstrdup("Indirect groups can have at most one bucket.");

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 2ac11b1..04fc7e4 100644 (file)
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2165,7 +2165,7 @@ ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type,
         ds_put_cstr(s, "bucket=");
 
         ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version);
-        if (bucket->weight != 1) {
+        if (bucket->weight != (type == OFPGT11_SELECT ? 1 : 0)) {
             ds_put_format(s, "weight:%"PRIu16",", bucket->weight);
         }
         if (bucket->watch_port != OFPP_NONE) {

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 3722bf1..7ddf88a 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -7580,7 +7580,8 @@ parse_ofp15_group_bucket_prop_watch(const struct ofpbuf *payload,
 
 static enum ofperr
 ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t buckets_length,
-                           enum ofp_version version, struct ovs_list *buckets)
+                           enum ofp_version version, uint8_t group_type,
+                           struct ovs_list *buckets)
 {
     struct ofp15_bucket *ob;
 
@@ -7593,7 +7594,7 @@ ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t buckets_length,
         size_t ob_len, actions_len, properties_len;
         ovs_be32 watch_port = ofputil_port_to_ofp11(OFPP_ANY);
         ovs_be32 watch_group = htonl(OFPG_ANY);
-        ovs_be16 weight = htons(1);
+        ovs_be16 weight = htons(group_type == OFPGT11_SELECT ? 1 : 0);
 
         ofpbuf_init(&ofpacts, 0);
 
@@ -7975,7 +7976,7 @@ ofputil_decode_ofp15_group_desc_reply(struct ofputil_group_desc *gd,
                      "bucket list length %u", bucket_list_len);
         return OFPERR_OFPBRC_BAD_LEN;
     }
-    error = ofputil_pull_ofp15_buckets(msg, bucket_list_len, version,
+    error = ofputil_pull_ofp15_buckets(msg, bucket_list_len, version, gd->type,
                                        &gd->buckets);
     if (error) {
         return error;
@@ -8273,7 +8274,7 @@ ofputil_pull_ofp15_group_mod(struct ofpbuf *msg, enum ofp_version ofp_version,
 
     bucket_list_len = ntohs(ogm->bucket_array_len);
     error = ofputil_pull_ofp15_buckets(msg, bucket_list_len, ofp_version,
-                                       &gm->buckets);
+                                       gm->type, &gm->buckets);
     if (error) {
         return error;
     }
@@ -8350,6 +8351,10 @@ ofputil_decode_group_mod(const struct ofp_header *oh,
     }
 
     LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
+        if (bucket->weight && gm->type != OFPGT11_SELECT) {
+            return OFPERR_OFPGMFC_INVALID_GROUP;
+        }
+
         switch (gm->type) {
         case OFPGT11_ALL:
         case OFPGT11_INDIRECT: