[ovs-dev] [Single DP 05/15] Allow the OpenFlow port to be requested for a port.

[Justin Pettit]

On Oct 18, 2012, at 9:30 PM, Ben Pfaff <blp at nicira.com> wrote:

> The new comment on the ->port_add member function of ofproto_class
> implies that the caller might pass in a NULL 'ofp_portp'.  I don't think
> that the implementation should have to bother with that possibility,
> because ofproto_port_add(), by design, always passes a non-null value,
> so I'd revise the comment.

Updated.

> struct if_cfg, in bridge.c, now ends with a spurious blank line.

Whoops.

> It looks like the signed 64-bit value from the database gets truncated
> down to an unsigned 16-bit value in iface_do_create().  Actually in
> iface_create() too, in the initialization of 'ofp_port'.  I'd check that
> it's in the valid range [1,OFPP_MAX) and use OFPP_NONE if not.

As you suggested later, I defined constraints in the database definition.  Is that sufficient?

> In iface_create(), the variable name 'result' is fairly vague, maybe
> 'ok' instead?

'ok'

> In vswitch.xml, I'm not sure that it makes sense to say that the port
> number can be greater than 0xff00.  I think that OpenFlow reserves
> those.  I guess I'd be inclined to say something more like:
> 
>        <p>Requested OpenFlow port number for this interface.  The port
>        number must be between 1 and 65279, inclusive.  Some datapaths
>        cannot satisfy all requests for particular port numbers.  When
>        this column is empty or the request cannot be fulfilled, the
>        system will choose a free port.  The <ref column="ofport"/>
>        column reports + the assigned OpenFlow port number.</p>

Sounds good.

> One could limit the allowed values at the database level, in the schema
> itself.

I didn't originally do this, since the limit varies by datapath implementation.  However, as you noted, there are real limits defined by OpenFlow, so I went ahead and put them in there.

> I think that this implementation generally considers the ofport
> request_value only if it is set in the same transaction that creates the
> port.  That is, changing ofport_request, or setting it when it was
> initially unset, will not change the OpenFlow port number.  Perhaps this
> should be noted in vswitch.xml too.


Done.

--Justin