[ovs-dev] [PATCH 06/24] ofp-util: Allow encoding of Open Flow 1.1 & 1.2 Barrier Request Messages

[Simon Horman]

On Tue, Jul 24, 2012 at 05:06:38PM -0700, Ben Pfaff wrote:
> On Wed, Jul 25, 2012 at 09:01:10AM +0900, Simon Horman wrote:
> > On Tue, Jul 24, 2012 at 01:31:41PM -0700, Ben Pfaff wrote:
> > > On Mon, Jul 23, 2012 at 03:16:35PM +0900, Simon Horman wrote:
> > > > Signed-off-by: Simon Horman <horms at verge.net.au>
> > > >  struct ofpbuf *
> > > > -ofputil_encode_barrier_request(void)
> > > > +ofputil_encode_barrier_request(uint8_t ofp_version)
> > > >  {
> > > > -    return ofpraw_alloc(OFPRAW_OFPT10_BARRIER_REQUEST, OFP10_VERSION, 0);
> > > > +    enum ofpraw type;
> > > > +
> > > > +    switch (ofp_version) {
> > > > +    case OFP12_VERSION:
> > > > +    case OFP11_VERSION:
> > > > +        type = OFPRAW_OFPT11_BARRIER_REQUEST;
> > > > +        break;
> > > > +
> > > > +    case OFP10_VERSION:
> > > > +        type = OFPRAW_OFPT10_BARRIER_REQUEST;
> > > > +        break;
> > > > +
> > > > +    default:
> > > > +        NOT_REACHED();
> > > > +    }
> > > > +
> > > > +    return ofpraw_alloc(type, ofp_version, 0);
> > > >  }
> > > 
> > > This will need to change when we add new OpenFlow versions, but I
> > > don't expect that new OpenFlow versions will actually change anything
> > > in the barrier request.  So I'd be inclined to do something like:
> > >         raw = (version == OFP10_VERSION
> > >                ? OFPRAW_OFPT10_BARRIER_REQUEST
> > >                : OFPRAW_OFPT11_BARRIER_REQUEST);
> > > instead of a switch that definitely needs to be updated.
> > 
> > I do have a slight preference for the case style, but I'll change
> > update your patch as per your suggestion.
> 
> So, here's another option that I've been thinking about anyway:
> instead of using macros for OFP10_VERSION etc., define an "enum
> ofp_version" with those as enumeration constants.  Then as long as we
> passed in the ofp_version as an enum ofp_version, we'd automatically
> get compiler warnings whenever we add a new version.
> 
> If you are willing to write up that change then I'd be happy to use a
> switch statement here.

I think that is an excellent idea.
I'll see about making a patch.