On Tue, Jul 28, 2015 at 10:05:07PM -0700, Ben Pfaff wrote:
> 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 <b...@nicira.com>
> Acked-by: Flavio Leitner <f...@sysclose.org>
So, the default for "select" remains the same, but for others it's
now zero. The change isn't a problem because for other kinds of
groups weight isn't defined, so it must return an error which is
included in this patch as well.
And with this patch: 1665 tests were successful.
Although my ack is above, ack again ;)
fbl
> ---
> lib/ofp-parse.c | 78
> ++++++++++++++++++++++++++++-----------------------------
> lib/ofp-print.c | 2 +-
> lib/ofp-util.c | 13 +++++++---
> 3 files changed, 49 insertions(+), 44 deletions(-)
>
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 8cdda50..ade664b 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -1143,7 +1143,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;
> @@ -1151,7 +1151,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;
> @@ -1333,40 +1333,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;
> @@ -1441,9 +1420,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");
> @@ -1504,12 +1480,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 44f3115..b8088f3 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -2376,7 +2376,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 9996e84..0d476dc 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -7822,7 +7822,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;
>
> @@ -7835,7 +7836,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);
>
> @@ -8217,7 +8218,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;
> @@ -8515,7 +8516,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;
> }
> @@ -8592,6 +8593,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:
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
