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>
}
static char * OVS_WARN_UNUSED_RESULT
}
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;
enum ofputil_protocol *usable_protocols)
{
char *pos, *key, *value;
struct ds actions;
char *error;
struct ds actions;
char *error;
+ bucket->weight = group_type == OFPGT11_SELECT ? 1 : 0;
bucket->bucket_id = OFPG15_BUCKET_ALL;
bucket->watch_port = OFPP_ANY;
bucket->watch_group = OFPG11_ANY;
bucket->bucket_id = OFPG15_BUCKET_ALL;
bucket->watch_port = OFPP_ANY;
bucket->watch_group = OFPG11_ANY;
*usable_protocols = OFPUTIL_P_OF11_UP;
*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;
+ /* 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;
for (name = strtok_r(string, "=, \t\r\n", &save_ptr); name;
name = strtok_r(NULL, "=, \t\r\n", &save_ptr)) {
char *value;
goto out;
}
had_type = true;
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");
} else if (!strcmp(name, "selection_method")) {
if (!(fields & F_GROUP_TYPE)) {
error = xstrdup("selection method is not needed");
- /* 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;
}
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.");
}
if (gm->type == OFPGT11_INDIRECT && !list_is_short(&gm->buckets)) {
error = xstrdup("Indirect groups can have at most one bucket.");
ds_put_cstr(s, "bucket=");
ofp_print_bucket_id(s, "bucket_id:", bucket->bucket_id, ofp_version);
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) {
ds_put_format(s, "weight:%"PRIu16",", bucket->weight);
}
if (bucket->watch_port != OFPP_NONE) {
static enum ofperr
ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t buckets_length,
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;
{
struct ofp15_bucket *ob;
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);
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);
ofpbuf_init(&ofpacts, 0);
"bucket list length %u", bucket_list_len);
return OFPERR_OFPBRC_BAD_LEN;
}
"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;
&gd->buckets);
if (error) {
return error;
bucket_list_len = ntohs(ogm->bucket_array_len);
error = ofputil_pull_ofp15_buckets(msg, bucket_list_len, ofp_version,
bucket_list_len = ntohs(ogm->bucket_array_len);
error = ofputil_pull_ofp15_buckets(msg, bucket_list_len, ofp_version,
+ gm->type, &gm->buckets);
if (error) {
return error;
}
if (error) {
return error;
}
}
LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
}
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:
switch (gm->type) {
case OFPGT11_ALL:
case OFPGT11_INDIRECT: