On Wed, Nov 12, 2014 at 10:03:24AM -0800, Ben Pfaff wrote:
> 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 <simon.hor...@netronome.com>
> >
> > ---
> > 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. */
Thanks will do.
I also plan to add the following similar text above
copy_buckets_for_remove_bucket().
/* Appends all of the a copy of all the buckets from 'ofgroup' to 'new_ofgroup'
* with the exception of the bucket whose bucket id is 'command_bucket_id'.
* Special 'command_bucket_id' values OFPG15_BUCKET_FIRST, OFPG15_BUCKET_LAST
* and OFPG15_BUCKET_ALL 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.
Yes, thanks for noticing that. I will fix that problem.
> 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. I thought I had removed that but it seems to have
slipped through somehow. I will remove it.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
