[ovs-dev] [of1.1 draft v2 1/7] Introduce ofputil_protocol, to abstract the protocol in use on a connection.

Simon Horman horms at verge.net.au
Tue Feb 14 16:33:57 PST 2012


On Tue, Feb 14, 2012 at 10:23:39AM -0800, Ben Pfaff wrote:
> On Mon, Feb 13, 2012 at 11:44:09AM +0900, Simon Horman wrote:
> > On Mon, Feb 06, 2012 at 09:16:48AM -0800, Ben Pfaff wrote:
> > > Thank you for the review.
> > > 
> > > On Mon, Feb 06, 2012 at 10:12:20AM +0900, Simon Horman wrote:
> > > > On Fri, Feb 03, 2012 at 12:43:49PM -0800, Ben Pfaff wrote:
> > > > > Open vSwitch already handles a few different protocol variations, but it
> > > > > does so in a nonuniform manner:
> > > > > 
> > > > >   - OpenFlow 1.0 and NXM flow formats are distinguished using the NXFF_*
> > > > >     constant values from nicira-ext.h.
> > > > > 
> > > > >   - The "flow_mod_table_id" feature setting is maintained in ofproto as
> > > > >     part of an OpenFlow connection's (ofconn's) state.
> > > > > 
> > > > > There's no way to easily communicate this state among components.  It's
> > > > > not much of a problem yet, but as more protocol support is added it seems
> > > > > better to have an abstract, uniform way to represent protocol versions and
> > > > > variants.  This commit implements that by introducing a new type
> > > > > "enum ofputil_protocol".  Each ofputil_protocol value represents a variant
> > > > > of a protocol version.  Each value is a separate bit, so a single enum
> > > > > can also represent a set of protocols, which is often useful as well.
> > > 
> > > [snip]
> > > 
> > > > > +        for (i = 0; i < CHAR_BIT * sizeof(enum ofputil_protocol); i++) {
> > > > > +            enum ofputil_protocol bit = 1u << i;
> > > > > +
> > > > > +            if (protocols & bit) {
> > > > > +                ds_put_cstr(&s, ofputil_protocol_to_string(bit));
> > > > 
> > > > Do you need to check if the return value of ofputil_protocol_to_string()
> > > > is NULL?
> > > 
> > > I don't think so.  ofputil_protocol_to_string() documents that it
> > > returns the name for any "single protocol" and its implementation has
> > > a switch statement that ensures that, if we add more protocols, then
> > > forgetting to add to the switch statement will provoke a warning.
> > > (But thanks for asking, it took me a minute to decide.)
> > 
> > True.
> > 
> > I might feel slightly more comfortable if there was a variant
> > of ofputil_protocol_to_string() that only handled single protocol names
> > and never returned NULL. But thats more a matter of taste than anything
> > else.
> 
> It sounds reasonable, but I'm not sure that it would really be more
> useful than the current function, which after all never returns NULL
> if there is only a single protocol involved.  It would be a special
> case of the existing function.

I agree that its usefulness is questionable :)



More information about the dev mailing list