Re: [libvirt] [PATCH v5] openvswitch: Add new port VLAN mode "802.1ad"

On Fri, Mar 08, 2019 at 11:21:37AM -0500, Laine Stump wrote: > On 3/8/19 8:34 AM, John Ferlan wrote: >> On 2/24/19 9:15 AM, ZhiPeng LU wrote: >>> This patch adds functionality to allow libvirt to configure the '802.1ad' >>> modes(802.1ad double-tagged) on openvswitch networks. >>> For example: >>> <interface type='bridge'> >>> <mac address='2c:da:41:1d:05:42'/> >>> <source bridge='ovs0'/> >>> <vlan> >>> <tag id='41' nativeMode='dot1q-tunnel'/> >>> </vlan> >>> <virtualport type='openvswitch'> >>> <parameters interfaceid='6401a152-0b99-40b5-92be-858810aa6d37'/> >>> </virtualport> >>> <model type='virtio'/> >>> <driver name='vhost'/> >>> <alias name='net0'/> >>> </interface> >>> >>> Signed-off-by: ZhiPeng Lu <luzhipeng(a)uniudc.com&gt; >>> --- >>> v1->v2: >>> 1. Fix "make syntax-check" failure >>> v2->v3: >>> 1. remove other_config when updating vlan >>> v3->v4: >>> 1. add commit message that has a brief description of the new >>> feature >>> 2. add tests for 'dot1q-tunnel' vlan mode >>> v4->v5: >>> 1. modify some description and format >>> >>> v4-resend: >>> https://www.redhat.com/archives/libvir-list/2019-February/msg00988.html >>> >>> docs/formatdomain.html.in | 33 +++++++++++++++------- >>> docs/formatnetwork.html.in | 26 ++++++++++------- >>> docs/schemas/networkcommon.rng | 1 + >>> src/conf/netdev_vlan_conf.c | 2 +- >>> src/util/virnetdevopenvswitch.c | 7 +++++ >>> src/util/virnetdevvlan.h | 1 + >>> tests/networkxml2xmlin/openvswitch-net.xml | 9 ++++++ >>> tests/networkxml2xmlout/openvswitch-net.xml | 9 ++++++ >>> .../openvswitch-net-modified.xml | 9 ++++++ >>> .../openvswitch-net-more-portgroups.xml | 9 ++++++ >>> .../openvswitch-net-without-alice.xml | 9 ++++++ >>> 11 files changed, 94 insertions(+), 21 deletions(-) >>> >> Apart from now needing to indicate support in 5.2.0 for the >> format*.html.in files and the need for a docs/news.xml note this seems >> fine to me and covers what Laine had originally reviewed. I can modify >> those two before pushing. >> >> Also, I've CC'd Laine in hopes he can also take a look for sanity's sake >> to ensure I didn't misinterpret something he requested previously! > > Yeah, sorry I haven't responded to the last couple revisions of this patch. > When I saw them I tagged them in red in my mail, but don't have an effective > queuing mechanism and ended up getting lost in some other distraction and > not getting back until the red message was scrolled way up out of sight :-/ > Anyway, thanks to John for keeping track of it and reviewing it, and ZhiPeng > Lu for being patient. > > > >> I'll also add a followup patch to update docs/news.xml with the >> following text: >> >> + <change> >> + <summary> >> + Add support for "802.1ad" VLAN mode > > (You know, I expected someone to counter-propose use of a non-official term > for this, since some places libvirt uses official names from the standards > documents and other places it uses informal terms. Since there was no > counter-proposal, I'm now unsure if that happened because 1) everyone agrees > with using "802.1ad" (which is unambiguous but its function may be less > obvious to a casual user), or 2) nobody even noticed :-P (but I still think > using the official name is better, especially because it assures we won't > end up with confusion if we later need to add some other sort of > tunneled/nested tagging) I would say 2) is the case :). Is there a standard for the two existing modes? To not make it even more confusing how about we uses the non-official term as the libvirt accepted value but in the docs we also mention the specific standard for each mode to make it absolutely clear what it refers to?