[ovs-dev] [PATCH v4] Add configurable OpenFlow port name.

[Xiao Liang]

Thanks for your review!

On Sat, Jun 4, 2016 at 1:15 AM, Ben Pfaff <blp at ovn.org> wrote:
> On Tue, May 24, 2016 at 03:07:23PM +0800, Xiao Liang wrote:
>> Add new column "ofname" in Interface table to configure port name reported
>> to controllers with OpenFlow protocol, thus decouple OpenFlow port name from
>> device name.
>>
>> For example:
>>     # ovs-vsctl set Interface eth0 ofname=wan
>>     # ovs-vsctl set Interface eth1 ofname=lan0
>> then controllers can recognize ports by their names.
>>
>> Signed-off-by: Xiao Liang <shaw.leon at gmail.com>
>> ---
>> v2: Added test for ofname
>>     Increased db schema version
>>     Updated NEWS
>> v3: Rebase
>> v4: Rebase
>>     Move prototypes in ofproto/ofproto.h to correct section
>
> Thanks for the patch!  I think that this is a reasonable approach.
>
> I'd like ovs-ofctl to report an error if a user attempts to refer to a
> port by name but there is more than one port with the name.  That could
> be in a separate patch.

And what do you think of also adding an option (say, "--all") to
select all ports with the name?

>
> The logic in ofproto_port_set_ofpname() does not make sense to me in the
> third case here:
>
>     if (!devname ||
>         (ofp_name && !strncmp(ofp_name, ofport->pp.name, name_size - 1)) ||
>         (!ofp_name && !strncmp(devname, ofport->pp.name, name_size - 1))) {
>         /* No need to change port name. */
>         return;
>     }
>
> I believe that in the
>         (!ofp_name && !strncmp(devname, ofport->pp.name, name_size - 1))) {
> case, we should remove any current ofp_name and change the name of the
> OpenFlow port back to the device name, but instead the code appears to
> do nothing.

In this case, the OpenFlow port name is already the same as devname
(strncmp). I think it's not necessary to remove the smap entry here
because it would be overwritten if changed later.

>
> It is too bad that the ofp_names smap exists at all.  It seems that it
> should be possible to simply represent the ofp_name inside the ofport,
> without the need for a separate map.

I don't like the smap, too. But didn't find a good way to get rid of
it. In update_port, ofproto_port and ofputil_phy_port are filled with
information from dpif. There seems no way to pass ofp_name here except
embedding it in dpif. So I followed the way of ofp_requests, putting
it into a map. Do you have any suggestions? And please correct me if
I'm wrong.

>
> It would be a good idea to mention the possibility of duplicate names in
> the documentation in vswitch.xml.
>

Yes, I will add this in next version.

Thanks,
Xiao