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.)
> > static void
> > -print_packet_list(struct list *packets)
> > +do_parse_flows__(struct ofputil_flow_mod *fms, size_t n_fms)
> > {
> > - struct ofpbuf *packet, *next;
> > + enum ofputil_protocol usable_protocols;
> > + enum ofputil_protocol protocol = 0;
> > + char *usable_s;
> > + size_t i;
> > +
> > + usable_protocols = ofputil_flow_mod_usable_protocols(fms, n_fms);
> > + usable_s = ofputil_protocols_to_string(usable_protocols);
> > + printf("usable protocols: %s\n", usable_s);
> > + free(usable_s);
> > +
> > + if (!(usable_protocols & allowed_protocols)) {
> > + ovs_fatal(0, "no usable protocol");
> > + }
> > + for (i = 0; i < sizeof(enum ofputil_protocol) * CHAR_BIT; i++) {
> > + protocol = 1 << i;
> > + if (protocol & usable_protocols & allowed_protocols) {
> > + break;
> > + }
> > + }
> > +
> > + printf("chosen protocol: %s\n", ofputil_protocol_to_string(protocol));
>
> And here
Similarly here we also know that 'protocol' has to be exactly one
bit. It's less obvious here, though, so I'll add an
"assert(IS_POW2(protocol));" below the loop.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
