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 :)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
