Thanks for your review!
On Sat, Jun 4, 2016 at 1:15 AM, Ben Pfaff <b...@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.l...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
