[ovs-dev] [more wdp 3/3] ofproto: Add Nicira extension to OpenFlow multiple flow tables support.

[Ben Pfaff]

On Thu, Jul 22, 2010 at 12:19:11AM -0700, Justin Pettit wrote:
> On Jul 12, 2010, at 5:14 PM, Ben Pfaff wrote:
> 
> > +/* This command enables or disables an Open vSwitch extension that allows a
> > + * controller to specify the OpenFlow table to which a flow should be added,
> > + * instead of having the switch decide which table is most appropriate as
> > + * required by OpenFlow 1.0.
> 
> I think it would be useful to indicate what the default setting is.

OK, I added "By default, the extension is disabled."

> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 8ab2bec..c788302 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -1067,6 +1068,7 @@ ofproto_add_flow(struct ofproto *p, const flow_t *flow,
> 
> > ofproto_delete_flow(struct ofproto *ofproto, const flow_t *flow)
> 
> It may be nice to have a comment that briefly describes that
> ofproto_add_flow() and ofproto_delete_flow() are functions intended
> for local ofproto users to trivially add flows to the flow table.
> Otherwise, to the occasional reader it's just a bit confusing, since
> there are also functions such as add_flow() in the same file, and the
> difference is not immediately obvious.

Yes, good point, I added some comments.

> > @@ -2266,7 +2285,15 @@ handle_flow_mod(struct ofproto *p, struct ofconn *ofconn,
> > 
> > +    switch (htons(ofm->command) & 0xff) {
> 
> Not that it really matters on most architectures, but you probably
> want to htons() the 0xff or else use ntohs().

Oops, that was meant to be ntohs(), thanks.

> > static int
> > +handle_flow_mod_table_id(struct ofconn *ofconn, struct nxt_tun_id_cookie *msg)
> 
> Shouldn't that be "nxt_flow_mod_table_id" instead of "nxt_tun_id_cookie"?

Ha, cut and paste error.  Thanks.

I pushed this, but I don't know how to properly test it.  Do you have an
idea?