[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
Mon Feb 13 02:44:09 UTC 2012


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.



More information about the dev mailing list