On 2/14/19 7:53 AM, 芦志朋 wrote: > This patch adds functionality to allow libvirt to configure the > 'dot1q-tunnel' > modes(802.1ad double-tagged) on openvswitch networks. no need for CRLF after 'dot1q-tunnel'.... also this will need to be fixed to match code and comments below. > 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'/> see my note below about interfaceid... this one is just short enough it seems... > </virtualport> > <model type='virtio'/> > <driver name='vhost'/> > <alias name='net0'/> > </interface> > > Signed-off-by: ZhiPeng Lu <luzhipeng@xxxxxxxxxx> > --- > 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 > Nice to also add a pointer to the history of where the previous patch come from in the archive, such as: v4-resend: https://www.redhat.com/archives/libvir-list/2018-December/msg00244.html v4: https://www.redhat.com/archives/libvir-list/2018-December/msg00197.html v3-resend: https://www.redhat.com/archives/libvir-list/2018-December/msg00044.html v3: https://www.redhat.com/archives/libvir-list/2018-December/msg00001.html NB: The only one to get at least some review it seems v2: https://www.redhat.com/archives/libvir-list/2018-November/msg01112.html v1: https://www.redhat.com/archives/libvir-list/2018-November/msg01027.html Note that once your updated patch contains a link to the previous one, then the next one would only need to contain one link. Allows future reviewer to see history just in case it's not reviewed by the same person who did the first review. > docs/formatdomain.html.in | 20 > ++++++++++++++------ > docs/formatnetwork.html.in | 20 > +++++++++++--------- > 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, 80 insertions(+), 16 deletions(-) > First off - it doesn't seem git send-email was used to generate the patch and your client has done some less than nice things to the generated patch. I'll just comment here inline, but hopefully your next posting can use the proper git send-email. I'm not the expert in this area, I would suspect Laine would know better than I do... I CC'd him mainly to keep him in the loop in case he missed this posting... I also note that the last time you posted this and it got reviewed you used "dot1q-tunnel" Laine pointed out you should use the official name; however, you just reposted with the common nickname. Since "dot1q-tunnel" wasn't acceptable and you put asked about using "802.1ad", then I think that's what you should have gone with in this update. I do see that src/util/virnetdevvportprofile.c has virNetDevVPort with a couple of "802.1Q*" strings, so there is precedent... There's also virInterfaceDefParseBondMode that converts "802.3ad"... Use those as guides for your naming. I also see your examples use "<parameters profileid='8021ad-profile'/>"... The profileid is not explained, but reading the networkcommon.rng file, I see it can be a string of up to 39 characters. Not quite sure how it's used, but perhaps we can also take this opportunity to add a patch that would explain it prior to this patch that's adding more consumers. > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 2ae5006..2a53d7c 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -6085,8 +6085,14 @@ qemu-kvm -net nic,model=? /dev/null > <b><tag id='42'/></b> > <b><tag id='123' nativeMode='untagged'/></b> > <b></vlan></b> > - ... ^^^^ This removal seems spurious > </interface> > + <interface type='bridge'> > + <b><vlan trunk='yes'></b> > + <b><tag id='42'/></b> > + <b><tag id='123' nativeMode='dot1q-tunnel'/></b> Rather than pure copy-n-paste, change 123 to some other number since 123 is associated with untagged AFAIUI > + <b></vlan></b> > + </interface> > +... This one should be indented by a few more spaces to align with the </interface> and it should be above that line as well since using ... indicates there's more XML that would be present, but all the snippet is doing it showing the important piece. > </devices> > ...</pre> > > @@ -6122,11 +6128,13 @@ qemu-kvm -net nic,model=? /dev/null > </p> > <p> > For network connections using Open vSwitch it is also possible > - to configure 'native-tagged' and 'native-untagged' VLAN modes > - <span class="since">Since 1.1.0.</span> This is done with the > - optional <code>nativeMode</code> attribute on > - the <code><tag></code> subelement: <code>nativeMode</code> > - may be set to 'tagged' or 'untagged'. The <code>id</code> > + to configure 'native-tagged' and 'native-untagged' > + <span class="since">Since 1.1.0.</span> and 'dot1q-tunnel' > + (802.1ad double-tagged) <span class="since">Since 5.0.0.</span> > + VLAN modes This is done with the optional <code>nativeMode</code> > + attribute on the <code><tag></code> subelement: > + <code>nativeMode</code> may be set to 'tagged' or 'untagged' or > + 'dot1q-tunnel'. The <code>id</code> Reading the above is getting confusing, can we go with: <p> For network connections using Open vSwitch it is also possible to configure the following VLAN modes: </p> <ul> <li>'tagged' <span class="since">Since 1.1.0</span> </li> <li>'untagged' <span class="since">Since 1.1.0</span> </li> <li>'802.1ad' <span class="since">Since 5.1.0</span> </li> </ul> <p> This is done with the optional <code>nativeMode</code> attribute on the <code><tag></code> subelement. The attribute may be set to a string from the above list. The <code>id</code> attribute of the <code><tag></code> subelement containing <code>nativeMode</code> sets which VLAN is considered to be the "native" VLAN for this interface and the <code>nativeMode</code> attribute determines whether or not traffic for that VLAN will be tagged. </p> NB: the formatnetwork.html.in is very similar to what you had here; however, in that hunk you added "or QinQ" to the last sentence. That probably should be done here. However, seeing QinQ is "odd" for this less knowledgeable reader - perhaps expand to "Q-in-Q" or "Q-in-Q tunneling" or perhaps "802.1ad double tagged" or something similar (I *can* google ;-)) > attribute of the <code><tag></code> subelement > containing <code>nativeMode</code> sets which VLAN is considered > to be the "native" VLAN for this interface, and > diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in > index 509cca9..cfce4a1 100644 > --- a/docs/formatnetwork.html.in > +++ b/docs/formatnetwork.html.in > @@ -705,16 +705,18 @@ > </p> > <p> > For network connections using Open vSwitch it is also possible > - to configure 'native-tagged' and 'native-untagged' VLAN modes > - <span class="since">Since 1.1.0.</span> This is done with the > - optional <code>nativeMode</code> attribute on > - the <code><tag></code> subelement: <code>nativeMode</code> > - may be set to 'tagged' or 'untagged'. The <code>id</code> > - attribute of the <code><tag></code> subelement > - containing <code>nativeMode</code> sets which VLAN is considered > - to be the "native" VLAN for this interface, and > + to configure 'native-tagged' and 'native-untagged' > + <span class="since">Since 1.1.0.</span> > + and 'dot1q-tunnel'(802.1ad double-tagged) VLAN modes. > + <span class="since">Since 5.0.0.</span> This is done with the > + optional <code>nativeMode</code> attribute on the > + <code><tag></code> subelement: <code>nativeMode</code> > + may be set to 'tagged' or 'untagged' or 'dot1q-tunnel'. > + The <code>id</code> attribute of the <code><tag></code> > + subelement containing <code>nativeMode</code> sets which VLAN is > + considered to be the "native" VLAN for this interface, and > the <code>nativeMode</code> attribute determines whether or not > - traffic for that VLAN will be tagged. > + traffic for that VLAN will be tagged or QinQ. See above - this should change similarly I believe. > </p> > <p> > <code><vlan></code> elements can also be specified in > diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng > index 2699555..11c48ff 100644 > --- a/docs/schemas/networkcommon.rng > +++ b/docs/schemas/networkcommon.rng > @@ -223,6 +223,7 @@ > <choice> > <value>tagged</value> > <value>untagged</value> > + <value>dot1q-tunnel</value> 802.1ad searches on 802 in doc/schemas/*.rng finds 802.3ad, 802.1Qbg, 802.1Qbh > </choice> > </attribute> > </optional> > diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c > index 57d73ed..892d31c 100644 > --- a/src/conf/netdev_vlan_conf.c > +++ b/src/conf/netdev_vlan_conf.c > @@ -25,7 +25,7 @@ > #define VIR_FROM_THIS VIR_FROM_NONE > > VIR_ENUM_IMPL(virNativeVlanMode, VIR_NATIVE_VLAN_MODE_LAST, > - "default", "tagged", "untagged", > + "default", "tagged", "untagged", "dot1q-tunnel", "802.1ad" > ); > > int > diff --git a/src/util/virnetdevopenvswitch.c > b/src/util/virnetdevopenvswitch.c > index 4fa3a57..dd2aeac 100644 > --- a/src/util/virnetdevopenvswitch.c > +++ b/src/util/virnetdevopenvswitch.c > @@ -85,6 +85,11 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, > virNetDevVlanPtr virtVlan) > virCommandAddArg(cmd, "vlan_mode=native-untagged"); > virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag); > break; > + case VIR_NATIVE_VLAN_MODE_DOT1Q_TUNNEL: see below for new name... > + virCommandAddArg(cmd, "vlan_mode=dot1q-tunnel"); > + virCommandAddArg(cmd, "other_config:qinq-ethtype=802.1q"); > + virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag); > + break; > case VIR_NATIVE_VLAN_MODE_DEFAULT: > default: > break; > @@ -498,6 +503,8 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname, > "--", "--if-exists", "clear", "Port", ifname, > "tag", > "--", "--if-exists", "clear", "Port", ifname, > "trunk", > "--", "--if-exists", "clear", "Port", ifname, > "vlan_mode", > + "--", "--if-exists", "remove", "Port", ifname, > + "other_config", "qinq-ethtype", I have no idea if this is right, but have to assume it is... > "--", "--if-exists", "set", "Port", ifname, NULL); > > if (virNetDevOpenvswitchConstructVlans(cmd, virtVlan) < 0) > diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h > index 2a13759..9d0a2bd 100644 > --- a/src/util/virnetdevvlan.h > +++ b/src/util/virnetdevvlan.h > @@ -27,6 +27,7 @@ typedef enum { > VIR_NATIVE_VLAN_MODE_DEFAULT = 0, > VIR_NATIVE_VLAN_MODE_TAGGED, > VIR_NATIVE_VLAN_MODE_UNTAGGED, > + VIR_NATIVE_VLAN_MODE_DOT1Q_TUNNEL, This nomenclature would seemingly need to be changed too to be something like VIR_NATIVE_VLAN_MODE_8021AD, John > > VIR_NATIVE_VLAN_MODE_LAST > } virNativeVlanMode; > diff --git a/tests/networkxml2xmlin/openvswitch-net.xml > b/tests/networkxml2xmlin/openvswitch-net.xml > index 2f6084d..0952859 100644 > --- a/tests/networkxml2xmlin/openvswitch-net.xml > +++ b/tests/networkxml2xmlin/openvswitch-net.xml > @@ -30,4 +30,13 @@ > <parameters profileid='native-profile'/> > </virtualport> > </portgroup> > + <portgroup name='8021ad'> > + <vlan trunk='yes'> > + <tag id='555' nativeMode='dot1q-tunnel'/> > + <tag id='666'/> > + </vlan> > + <virtualport> > + <parameters profileid='8021ad-profile'/> > + </virtualport> > + </portgroup> > </network> > diff --git a/tests/networkxml2xmlout/openvswitch-net.xml > b/tests/networkxml2xmlout/openvswitch-net.xml > index 2f6084d..0952859 100644 > --- a/tests/networkxml2xmlout/openvswitch-net.xml > +++ b/tests/networkxml2xmlout/openvswitch-net.xml > @@ -30,4 +30,13 @@ > <parameters profileid='native-profile'/> > </virtualport> > </portgroup> > + <portgroup name='8021ad'> > + <vlan trunk='yes'> > + <tag id='555' nativeMode='dot1q-tunnel'/> > + <tag id='666'/> > + </vlan> > + <virtualport> > + <parameters profileid='8021ad-profile'/> > + </virtualport> > + </portgroup> > </network> > diff --git a/tests/networkxml2xmlupdateout/openvswitch-net-modified.xml > b/tests/networkxml2xmlupdateout/openvswitch-net-modified.xml > index cc0c344..e5794fc 100644 > --- a/tests/networkxml2xmlupdateout/openvswitch-net-modified.xml > +++ b/tests/networkxml2xmlupdateout/openvswitch-net-modified.xml > @@ -30,4 +30,13 @@ > <parameters profileid='native-profile'/> > </virtualport> > </portgroup> > + <portgroup name='8021ad'> > + <vlan trunk='yes'> > + <tag id='555' nativeMode='dot1q-tunnel'/> > + <tag id='666'/> > + </vlan> > + <virtualport> > + <parameters profileid='8021ad-profile'/> > + </virtualport> > + </portgroup> > </network> > diff --git > a/tests/networkxml2xmlupdateout/openvswitch-net-more-portgroups.xml > b/tests/networkxml2xmlupdateout/openvswitch-net-more-portgroups.xml > index 7c19ad9..3df0b96 100644 > --- a/tests/networkxml2xmlupdateout/openvswitch-net-more-portgroups.xml > +++ b/tests/networkxml2xmlupdateout/openvswitch-net-more-portgroups.xml > @@ -41,4 +41,13 @@ > <parameters profileid='native-profile'/> > </virtualport> > </portgroup> > + <portgroup name='8021ad'> > + <vlan trunk='yes'> > + <tag id='555' nativeMode='dot1q-tunnel'/> > + <tag id='666'/> > + </vlan> > + <virtualport> > + <parameters profileid='8021ad-profile'/> > + </virtualport> > + </portgroup> > </network> > diff --git > a/tests/networkxml2xmlupdateout/openvswitch-net-without-alice.xml > b/tests/networkxml2xmlupdateout/openvswitch-net-without-alice.xml > index 4104424..38846aa 100644 > --- a/tests/networkxml2xmlupdateout/openvswitch-net-without-alice.xml > +++ b/tests/networkxml2xmlupdateout/openvswitch-net-without-alice.xml > @@ -20,4 +20,13 @@ > <parameters profileid='native-profile'/> > </virtualport> > </portgroup> > + <portgroup name='8021ad'> > + <vlan trunk='yes'> > + <tag id='555' nativeMode='dot1q-tunnel'/> > + <tag id='666'/> > + </vlan> > + <virtualport> > + <parameters profileid='8021ad-profile'/> > + </virtualport> > + </portgroup> > </network> > -- > 1.8.3.1 > > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list >
