On Tue, Nov 11, 2014 at 08:49:56AM -0800, Ben Pfaff wrote:
> On Tue, Nov 11, 2014 at 12:39:19PM +0900, Simon Horman wrote:
> > This provides the bulk of the ofproto side of support for
> > OpenFlow 1.5 group messages. It provides for encoding and decoding
> > of updated group mod and group desc reply messages. This includes
> > a new bucket format and their properties.
> >
> > Open Flow 1.5 Groups also have properties but as no non-experimenter
> > properties are defined this patch does not provide parsing or encoding
> > of group properties.
> >
> > ONF-JIRA: EXT-350
> > Signed-off-by: Simon Horman <[email protected]>
> >
> > ---
> > v2
> > * Rebase to use actions_len field of struct ofp15_bucket
> > instead of action_list_len field
> > * As suggested by Ben Pfaff
> > - Use ONF-JIRA: EXT-350 annotation in changelog
> > - Squash "ofproto: Add OFPRAW_OFPT15_GROUP_MOD"
>
> Thanks! I folded in the following changes:
>
> - Make id_pool_destroy() tolerate null pointer instead of avoiding
> calling it with a null pointer (as suggested in CodingStyle).
>
> - Avoid worrying about 4 billion buckets, which isn't really possible.
>
> - ofpbuf_try_pull() already tolerates too little data and returns NULL
> in that case, so we don't need an extra check.
>
> - Fix memory leaks on error paths.
>
> - Fix indentation in ofputil_put_ofp15_bucket().
>
> - Fix type of 'bucket_id' in struct ofputil_bucket (reported by
> sparse).
>
> and applied the result to master.
Likewise, Thanks! Very much appreciated.
> diff --git a/lib/id-pool.c b/lib/id-pool.c
> index e671a9c..6b93d37 100644
> --- a/lib/id-pool.c
> +++ b/lib/id-pool.c
> @@ -51,8 +51,10 @@ id_pool_create(uint32_t base, uint32_t n_ids)
> void
> id_pool_destroy(struct id_pool *pool)
> {
> - id_pool_uninit(pool);
> - free(pool);
> + if (pool) {
> + id_pool_uninit(pool);
> + free(pool);
> + }
> }
>
> static void
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 520241e..496dc40 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -7284,38 +7284,38 @@ ofputil_put_ofp15_bucket(const struct ofputil_bucket
> *bucket,
> uint32_t bucket_id, enum ofp11_group_type
> group_type,
> struct ofpbuf *openflow, enum ofp_version
> ofp_version)
> {
> - struct ofp15_bucket *ob;
> - size_t start, actions_start, actions_len;
> + struct ofp15_bucket *ob;
> + size_t start, actions_start, actions_len;
>
> - start = ofpbuf_size(openflow);
> - ofpbuf_put_zeros(openflow, sizeof *ob);
> + start = ofpbuf_size(openflow);
> + ofpbuf_put_zeros(openflow, sizeof *ob);
>
> - actions_start = ofpbuf_size(openflow);
> - ofpacts_put_openflow_actions(bucket->ofpacts, bucket->ofpacts_len,
> - openflow, ofp_version);
> - actions_len = ofpbuf_size(openflow) - actions_start;
> + actions_start = ofpbuf_size(openflow);
> + ofpacts_put_openflow_actions(bucket->ofpacts, bucket->ofpacts_len,
> + openflow, ofp_version);
> + actions_len = ofpbuf_size(openflow) - actions_start;
>
> - if (group_type == OFPGT11_SELECT) {
> - ofputil_put_ofp15_group_bucket_prop_weight(htons(bucket->weight),
> - openflow);
> - }
> - if (bucket->watch_port != OFPP_ANY) {
> - ovs_be32 port = ofputil_port_to_ofp11(bucket->watch_port);
> - ofputil_put_ofp15_group_bucket_prop_watch(port,
> - OFPGBPT15_WATCH_PORT,
> - openflow);
> - }
> - if (bucket->watch_group != OFPG_ANY) {
> - ovs_be32 group = htonl(bucket->watch_group);
> - ofputil_put_ofp15_group_bucket_prop_watch(group,
> - OFPGBPT15_WATCH_GROUP,
> - openflow);
> - }
> + if (group_type == OFPGT11_SELECT) {
> + ofputil_put_ofp15_group_bucket_prop_weight(htons(bucket->weight),
> + openflow);
> + }
> + if (bucket->watch_port != OFPP_ANY) {
> + ovs_be32 port = ofputil_port_to_ofp11(bucket->watch_port);
> + ofputil_put_ofp15_group_bucket_prop_watch(port,
> + OFPGBPT15_WATCH_PORT,
> + openflow);
> + }
> + if (bucket->watch_group != OFPG_ANY) {
> + ovs_be32 group = htonl(bucket->watch_group);
> + ofputil_put_ofp15_group_bucket_prop_watch(group,
> + OFPGBPT15_WATCH_GROUP,
> + openflow);
> + }
>
> - ob = ofpbuf_at_assert(openflow, start, sizeof *ob);
> - ob->len = htons(ofpbuf_size(openflow) - start);
> - ob->actions_len = htons(actions_len);
> - ob->bucket_id = htonl(bucket_id);
> + ob = ofpbuf_at_assert(openflow, start, sizeof *ob);
> + ob->len = htons(ofpbuf_size(openflow) - start);
> + ob->actions_len = htons(actions_len);
> + ob->bucket_id = htonl(bucket_id);
> }
>
> static void
> @@ -7452,12 +7452,6 @@ ofputil_pull_ofp11_buckets(struct ofpbuf *msg, size_t
> buckets_length,
> return OFPERR_OFPGMFC_BAD_WATCH;
> }
> bucket->watch_group = ntohl(ob->watch_group);
> -
> - if (bucket_id > OFPG15_BUCKET_MAX) {
> - VLOG_WARN_RL(&bad_ofmsg_rl, "OpenFlow message with too many "
> - "buckets (%u)", bucket_id + 1);
> - return OFPERR_OFPGMFC_BAD_BUCKET;
> - }
> bucket->bucket_id = bucket_id++;
>
> bucket->ofpacts = ofpbuf_steal_data(&ofpacts);
> @@ -7475,7 +7469,7 @@ parse_ofp15_group_bucket_prop_weight(const struct
> ofpbuf *payload,
> struct ofp15_group_bucket_prop_weight *prop = ofpbuf_data(payload);
>
> if (ofpbuf_size(payload) != sizeof *prop) {
> - log_property(false, "Open flow bucket weight property length "
> + log_property(false, "OpenFlow bucket weight property length "
> "%u is not valid", ofpbuf_size(payload));
> return OFPERR_OFPBPC_BAD_LEN;
> }
> @@ -7492,7 +7486,7 @@ parse_ofp15_group_bucket_prop_watch(const struct ofpbuf
> *payload,
> struct ofp15_group_bucket_prop_watch *prop = ofpbuf_data(payload);
>
> if (ofpbuf_size(payload) != sizeof *prop) {
> - log_property(false, "Open flow bucket watch port or group "
> + log_property(false, "OpenFlow bucket watch port or group "
> "property length %u is not valid",
> ofpbuf_size(payload));
> return OFPERR_OFPBPC_BAD_LEN;
> }
> @@ -7510,7 +7504,7 @@ ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t
> buckets_length,
>
> list_init(buckets);
> while (buckets_length > 0) {
> - struct ofputil_bucket *bucket;
> + struct ofputil_bucket *bucket = NULL;
> struct ofpbuf ofpacts;
> enum ofperr err = OFPERR_OFPGMFC_BAD_BUCKET;
> struct ofpbuf properties;
> @@ -7521,9 +7515,7 @@ ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t
> buckets_length,
>
> ofpbuf_init(&ofpacts, 0);
>
> - ob = (buckets_length >= sizeof *ob
> - ? ofpbuf_try_pull(msg, sizeof *ob)
> - : NULL);
> + ob = ofpbuf_try_pull(msg, sizeof *ob);
> if (!ob) {
> VLOG_WARN_RL(&bad_ofmsg_rl, "buckets end with %"PRIuSIZE
> " leftover bytes", buckets_length);
> @@ -7620,15 +7612,17 @@ ofputil_pull_ofp15_buckets(struct ofpbuf *msg, size_t
> buckets_length,
>
> continue;
>
> -err:
> + err:
> + free(bucket);
> ofpbuf_uninit(&ofpacts);
> ofputil_bucket_list_destroy(buckets);
> return err;
> }
>
> if (ofputil_bucket_check_duplicate_id(buckets)) {
> - VLOG_WARN_RL(&bad_ofmsg_rl, "Duplicate bucket id");
> - return OFPERR_OFPGMFC_BAD_BUCKET;
> + VLOG_WARN_RL(&bad_ofmsg_rl, "Duplicate bucket id");
> + ofputil_bucket_list_destroy(buckets);
> + return OFPERR_OFPGMFC_BAD_BUCKET;
> }
>
> return 0;
> @@ -7805,11 +7799,7 @@ ofputil_encode_ofp15_group_mod(enum ofp_version
> ofp_version,
> }
>
> if (!id_pool_alloc_id(bucket_ids, &bucket_id)) {
> - VLOG_WARN_RL(&bad_ofmsg_rl,
> - "could not allocate group bucket id");
> - ofpbuf_delete(b);
> - b = NULL;
> - goto err;
> + OVS_NOT_REACHED();
> }
> } else {
> bucket_id = bucket->bucket_id;
> @@ -7824,10 +7814,7 @@ ofputil_encode_ofp15_group_mod(enum ofp_version
> ofp_version,
> ogm->command_bucket_id = htonl(gm->command_bucket_id);
> ogm->bucket_list_len = htons(ofpbuf_size(b) - start_ogm - sizeof *ogm);
>
> -err:
> - if (bucket_ids) {
> - id_pool_destroy(bucket_ids);
> - }
> + id_pool_destroy(bucket_ids);
> return b;
> }
>
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index 8777575..d85a255 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -986,7 +986,7 @@ struct ofputil_bucket {
> uint32_t watch_group; /* Group whose state affects whether this
> * bucket is live. Only required for fast
> * failover groups. */
> - ovs_be32 bucket_id; /* Bucket Id used to identify bucket*/
> + uint32_t bucket_id; /* Bucket Id used to identify bucket*/
> struct ofpact *ofpacts; /* Series of "struct ofpact"s. */
> size_t ofpacts_len; /* Length of ofpacts, in bytes. */
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
