On Wed, Nov 12, 2014 at 03:24:11PM +0900, Simon Horman wrote:
> Handle (draft) OpenFlow 1.5 insert and remove group commands
> of group mod messages.
>
> ONF-JIRA: EXT-350
> Signed-off-by: Simon Horman <[email protected]>
>
> ---
> v3
> * No change
> v2
> * As suggested by Ben Pfaff
> - Use ONF-JIRA: EXT-350 annotation in changelog
It took me a few minutes to figure out what
copy_buckets_for_insert_bucket() was doing. I suggest the following
comments (and an indentation fix):
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index aa6ed87..0f0bd25 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5795,6 +5795,11 @@ add_group(struct ofproto *ofproto, struct
ofputil_group_mod *gm)
return error;
}
+/* Adds all of the buckets from 'ofgroup' to 'new_ofgroup'. The buckets
+ * already in 'new_ofgroup' will be placed just after the (copy of the) bucket
+ * in 'ofgroup' with bucket ID 'command_bucket_id'. Special
+ * 'command_bucket_id' values OFPG15_BUCKET_FIRST and OFPG15_BUCKET_LAST are
+ * also honored. */
static enum ofperr
copy_buckets_for_insert_bucket(const struct ofgroup *ofgroup,
struct ofgroup *new_ofgroup,
@@ -5818,11 +5823,13 @@ copy_buckets_for_insert_bucket(const struct ofgroup
*ofgroup,
}
}
+ /* First copy buckets from 'ofgroup' to the end of the list in
+ * 'new_ofgroup'. */
ofputil_bucket_clone_list(&new_ofgroup->buckets, &ofgroup->buckets, NULL);
if (ofputil_bucket_check_duplicate_id(&ofgroup->buckets)) {
- VLOG_WARN_RL(&rl, "Duplicate bucket id");
- return OFPERR_OFPGMFC_BUCKET_EXISTS;
+ VLOG_WARN_RL(&rl, "Duplicate bucket id");
+ return OFPERR_OFPGMFC_BUCKET_EXISTS;
}
/* Rearrange list according to command_bucket_id */
I think that copy_buckets_for_remove_bucket() has a bug: shouldn't it
report OFPERR_OFPGMFC_UNKNOWN_BUCKET even if the group has no buckets?
Currently it does nothing.
I think that the special check in handle_group_mod() for a nonempty
bucket list in the OFPGC15_REMOVE_BUCKET case can be removed now because
it is now checked in ofp-util.c at decoding time.
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
