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