[ovs-dev] [PATCH v2 03/13] ofp-util: Encoding and decoding of (draft) OpenFlow 1.5 group messages
Ben Pfaff
blp at nicira.com
Tue Nov 11 16:49:56 UTC 2014
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 <simon.horman at netronome.com>
>
> ---
> 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.
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. */
More information about the dev
mailing list