On Mon, Nov 10, 2014 at 01:47:51PM +0900, Simon Horman wrote:
> EXT-350
> Signed-off-by: Simon Horman <simon.hor...@netronome.com>
Thanks, Simon!
I see some misspellings of OpenFlow as "OpenFLow". Please fix those.
> + OFPGC15_INSERT_BUCKET = 3,/* Insert action buckets to the already
> available
> + list of action buckets in a matching group */
> + OFPGC15_REMOVE_BUCKET = 5,/* Remove all action buckets or any specific
> + action bucket from matching group */
I don't know why there's a gap here (although it matches the spec).
I've asked Jean whether he could remove the gap by changing
OFPGC15_REMOVE_BUCKET to 4.
> +/* Common header for all group bucket properties. */
> +struct ofp15_group_bucket_prop_header {
> + ovs_be16 type; /* One of OFPGBPT15_*. */
> + ovs_be16 length; /* Length in bytes of this property. */
> +};
> +OFP_ASSERT(sizeof(struct ofp15_group_bucket_prop_header) == 4);
This is identical to ofp_prop_header (defined in ofp-util.c). We don't
usually make a new copy for each new kind of property.
> +/* Experimenter group bucket property */
> +struct ofp15_group_bucket_prop_experimenter {
> + ovs_be16 type; /* OFPGBPT15_EXPERIMENTER. */
> + ovs_be16 length; /* Length in bytes of this property. */
> + ovs_be32 experimenter; /* Experimenter ID which takes the same
> + form as in struct
> + ofp_experimenter_header. */
> + ovs_be32 exp_type; /* Experimenter defined. */
> + /* Followed by:
> + * - Exactly (length - 12) bytes containing the experimenter data, then
> + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7)
> + * bytes of all-zero bytes */
> + /* ovs_be32 experimenter_data[0]; */
> +};
> +OFP_ASSERT(sizeof(struct ofp15_group_bucket_prop_experimenter) == 12);
Are your remaining patches going to define any experimenter properties,
or maybe a framework for working with experimenter properties? For
other properties we haven't bothered with this structure until some
experimenter properties arise (I don't think any have yet).
> +/* Bucket for use in groups. */
> +struct ofp15_bucket {
> + ovs_be16 len; /* Length the bucket in bytes, including
> + this header and any padding to make it
> + 64-bit aligned. */
> + ovs_be16 action_list_len; /* Length of all actions in bytes. */
> + ovs_be32 bucket_id; /* Bucket Id used to identify bucket*/
> + /* Followed by exactly len - 8 bytes of group bucket properties. */
> + /* Followed by:
> + * - Exactly 'action_list_len' bytes containing an array of
> + * struct ofp_action_*.
> + * - Zero or more bytes of group bucket properties to fill out the
> + * overall length in header.length. */
> +};
> +OFP_ASSERT(sizeof(struct ofp15_bucket) == 8);
The above implies in a couple of places that a bucket contains an action
list. This is wrong; a bucket contains an action set.
> +/* Common header for all group properties. */
> +struct ofp15_group_prop_header {
> + ovs_be16 type; /* One of OFPGPT15_*. */
> + ovs_be16 length; /* Length in bytes of this property. */
> +};
> +OFP_ASSERT(sizeof(struct ofp15_group_prop_header) == 4);
Again I wouldn't define the above since ofp_prop_header should work.
> +/* Experimenter group property */
> +struct ofp15_group_prop_experimenter {
> + ovs_be16 type; /* OFPGPT15_EXPERIMENTER. */
> + ovs_be16 length; /* Length in bytes of this property. */
> + ovs_be32 experimenter; /* Experimenter ID which takes the same
> + form as in struct
> + ofp_experimenter_header. */
> + ovs_be32 exp_type; /* Experimenter defined. */
> + /* Followed by:
> + * - Exactly (length - 12) bytes containing the experimenter data, then
> + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7)
> + * bytes of all-zero bytes */
> + /* ovs_be32 experimenter_data[0]; */
> +};
> +OFP_ASSERT(sizeof(struct ofp15_group_prop_experimenter) == 12);
Again I wouldn't define this unless you're planning to do something with
it in later patches.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
