[ovs-dev] [PATCH 04/22] ofproto: Add types for (draft) OpenFlow 1.5 group mod

Simon Horman simon.horman at netronome.com
Tue Nov 11 01:01:03 UTC 2014


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 <simon.horman at netronome.com>
> 
> Thanks, Simon!
> 
> I see some misspellings of OpenFlow as "OpenFLow".  Please fix those.

Thanks, will do.

> 
> > +    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.

> > +/* 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.

Thanks, I will remove it.

> > +/* 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).

I do not have any plans to use bucket experimenter properties.
I will remove struct ofp15_group_bucket_prop_experimenter.

> 
> > +/* 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:

 /* 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_be16 actions_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
+     *   - Exactly 'actions_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. */

> > +/* 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.

Thanks, I will remove struct ofp15_group_prop_header.

> > +/* 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.



More information about the dev mailing list