On Mon, Nov 10, 2014 at 08:34:30PM -0800, Ben Pfaff wrote:
> On Tue, Nov 11, 2014 at 10:01:03AM +0900, Simon Horman wrote:
> > On Mon, Nov 10, 2014 at 09:55:53AM -0800, Ben Pfaff wrote:
> > > On Mon, Nov 10, 2014 at 01:47:51PM +0900, Simon Horman wrote:
> > > > EXT-350
> > > > Signed-off-by: Simon Horman <[email protected]>
> > > > + 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.
> >
> > Thanks, I am a little surprised by the gap too.
> > I'll leave the code as-is for now, so that it reflects the current
> > spec. And plan to update it if the spec is changed.
>
> After both of us talked to Jean, I see that the gap was intentional, so
> this doesn't need to change. Thanks!
>
> > >
> > > > +/* 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.
> >
> > I think this is another area where we should see about getting the spec
> > updated. In the mean time I'll change this code around as follows:
>
> Thanks for the update. (I see that Jean is planning to update the
> spec.)
Thanks. Once the change is decided on I'll update the code accordingly.
> > > > +/* 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.
> >
> > I am planning to do something with this in a subsequent series.
> > But as it is not used in this series I think it is best to remove
> > it from this patch.
>
> OK. Thank you. (I suspect that a generic ofp_prop_experimenter for
> experimenter property headers might work as well as the existing generic
> ofp_prop_header.)
Now that you mention it, yes that seems likely.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
