Re: [libvirt] [PATCHv3] Configure native vlan modes on Open vSwitch ports

On 04/30/2013 02:12 PM, james robson wrote: > This patch adds functionality to allow libvirt to configure the > 'native-tagged' and 'native-untagged' modes on openvswitch networks. > > v2 changes: > Fix problems reported by Eric Blake > > v3 changes: > Re work patch to address review comments > > --- > > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 888c005..5278534 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -3290,6 +3290,13 @@ qemu-kvm -net nic,model=? /dev/null > &lt;parameters > interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/&gt; > &lt;/virtualport&gt; > &lt;/interface&gt; > + &lt;interface type='bridge'&gt; > + <b>&lt;vlan trunk='yes'&gt;</b> > + <b>&lt;tag id='42'/&gt;</b> > + <b>&lt;tag id='123' nativeMode='untagged'/&gt;</b> > + <b>&lt;/vlan&gt;</b> > + ... > + &lt;/interface&gt; > &lt;devices&gt; > ...</pre> > > @@ -3316,6 +3323,15 @@ qemu-kvm -net nic,model=? /dev/null > vlan element. > </p> > > + <p> > + For network connections using openvswitch it is possible to > + configure the 'native-tagged' and 'native-untagged' vlan modes > + <span class="since">(Since 1.0.6).</span> This uses the optional > + <code>nativeMode</code> attribute on the <code>&lt;tag&gt;</code> > + element: <code>nativeMode</code> may be set to 'tagged' or > + 'untagged'. The id atribute of the element sets the native vlan. > + </p> > + > <h5><a name="elementLink">Modifying virtual link state</a></h5> > <pre> > ... > diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in > index 4dd0415..e065ca4 100644 > --- a/docs/formatnetwork.html.in > +++ b/docs/formatnetwork.html.in > @@ -414,6 +414,7 @@ > <span class="since">Since 0.9.4</span> > </p> > > + > <h5><a name="elementVlanTag">Setting VLAN tag (on supported network > types only)</a></h5> > > <pre> > @@ -429,6 +430,13 @@ > &lt;parameters > interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/&gt; > &lt;/virtualport&gt; > &lt;/interface&gt; > + &lt;interface type='bridge'&gt; > + <b>&lt;vlan trunk='yes'&gt;</b> > + <b>&lt;tag id='42'/&gt;</b> > + <b>&lt;tag id='123' nativeMode='untagged'/&gt;</b> > + <b>&lt;/vlan&gt;</b> > + ... > + &lt;/interface&gt; > &lt;devices&gt; > ...</pre> > > @@ -452,6 +460,14 @@ > be added to the vlan element. > </p> > <p> > + For network connections using openvswitch it is possible to > + configure the 'native-tagged' and 'native-untagged' vlan modes > + <span class="since">(Since 1.0.6).</span> This uses the optional > + <code>nativeMode</code> attribute on the <code>&lt;tag&gt;</code> > + element: <code>nativeMode</code> may be set to 'tagged' or > + 'untagged'. The id atribute of the element sets the native vlan. > + </p> > + <p> > <code>&lt;vlan&gt;</code> elements can also be specified in > a <code>&lt;portgroup&gt;</code> element, as well as directly in > a domain's <code>&lt;interface&gt;</code> element. In the case > diff --git a/docs/schemas/networkcommon.rng > b/docs/schemas/networkcommon.rng > index 51ff759..e60f1fc 100644 > --- a/docs/schemas/networkcommon.rng > +++ b/docs/schemas/networkcommon.rng > @@ -204,6 +204,14 @@ > <param name="maxInclusive">4095</param> > </data> > </attribute> > + <optional> > + <attribute name="nativeMode"> > + <choice> > + <value>tagged</value> > + <value>untagged</value> > + </choice> > + </attribute> > + </optional> > <empty/> > </element> > </oneOrMore> > diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c > index 13ba8c6..dd5b64e 100644 > --- a/src/conf/netdev_vlan_conf.c > +++ b/src/conf/netdev_vlan_conf.c > @@ -17,6 +17,7 @@ > * > * Authors: > * Laine Stump <laine(a)redhat.com&gt; > + * James Robson <jrobson(a)websense.com&gt; > */ > > #include <config.h> > @@ -33,6 +34,7 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr > ctxt, virNetDevVlanPtr de > int ret = -1; > xmlNodePtr save = ctxt->node; > const char *trunk = NULL; > + const char *nativeMode = NULL; > xmlNodePtr *tagNodes = NULL; > int nTags, ii; > > @@ -54,6 +56,8 @@ virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr > ctxt, virNetDevVlanPtr de > goto error; > } > > + def->nativeMode = 0; > + def->nativeTag = 0; > for (ii = 0; ii < nTags; ii++) { > unsigned long id; > > @@ -68,6 +72,21 @@ virNetDevVlanParse(xmlNodePtr node, > xmlXPathContextPtr ctxt, virNetDevVlanPtr de > _("vlan tag id %lu too large (maximum > 4095)"), id); > goto error; > } > + if ((nativeMode = virXPathString("string(./@nativeMode)", > ctxt)) != NULL) { > + if (STRCASENEQ(nativeMode, "tagged") && > STRCASENEQ(nativeMode, "untagged")) { > + virReportError(VIR_ERR_XML_ERROR, > + _("invalid \"nativeMode='%s'\" in <tag> > - " > + "native_mode must be 'tagged' or > 'untagged'"), nativeMode); > + goto error; > + } > + if (def->nativeMode != 0) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("duplicate native vlan setting")); > + goto error; > + } > + def->nativeMode = STRCASEEQ(nativeMode, "tagged") ? 1 : 2; We prefer for things like this to have an enum defined, and use the ${enum-type-name}Type(To|From)String to convert between the string and the numeric value. Then in the code that uses the value, we compare to the enum rather than a plain number. For example, you can search for virDomainHostdevSubsys in src/conf/domain_conf.[ch]. Basically you need: * in the .h file: typedef enum { VIR_WHATEVER_NAME_TYPE_DEFAULT = 0, VIR_WHATEVER_NAME_TYPE_ONE, VIR_WHATEVER_NAME_TYPE_ANOTHER, VIR_WHATEVER_NAME_TYPE_LAST } virWhateverNameType; VIR_ENUM_DECL(virWhateverName) * in the .c file: VIR_ENUM_IMPL(virWhateverName, VIR_WHATEVER_NAME_TYPE_LAST, "default", "one", "another") * at the place you parse: if ((def->whatever = virWhateverTypeFromString(type)) <= 0) { virReportError(.....); goto error/cleanup; } * at the place you format: if (def->whatever) { const char *whateverStr = virWhateverTypeToString(def->whatever); if (!whatever) { virReportError(VIR_ERR_INTERNAL_ERROR, "Bad whatever value"...); goto error/cleanup; } virAsprintf(buf, " whatever='%s', whateverStr); * in the code that implements it: switch (def->whatever) { case VIR_WHATEVER_NAME_TYPE_ONE: blah; break; case VIR_WHATEVER_NAME_TYPE_ANOTHER: bleh; break; case VIR_WHATEVER_NAME_TYPE_DEFAULT: default: break; } or something like that.

> + def->nativeTag = id; > + } > def->tag[ii] = id; > } > > @@ -89,6 +108,12 @@ virNetDevVlanParse(xmlNodePtr node, > xmlXPathContextPtr ctxt, virNetDevVlanPtr de > "is required for more than one vlan > tag"), trunk); > goto error; > } > + if (def->nativeMode != 0) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("invalid configuration in <vlan> - > \"trunk='no'\" is " > + "not allowed with a native vlan id")); > + goto error; > + } > /* allow (but discard) "trunk='no' if there is a single tag > */ > if (STRCASENEQ(trunk, "no")) { > virReportError(VIR_ERR_XML_ERROR, > @@ -125,7 +150,11 @@ virNetDevVlanFormat(virNetDevVlanPtr def, > virBufferPtr buf) > > virBufferAsprintf(buf, "<vlan%s>\n", def->trunk ? " trunk='yes'" : > ""); > for (ii = 0; ii < def->nTags; ii++) { > - virBufferAsprintf(buf, " <tag id='%u'/>\n", def->tag[ii]); > + if (def->nativeTag == def->tag[ii]) { > + virBufferAsprintf(buf, " <tag id='%u' nativeMode='% > s'/>\n", def->tag[ii], def->nativeMode == 1 ? "tagged" : "untagged"); > + } else { > + virBufferAsprintf(buf, " <tag id='%u'/>\n", def->tag[ii]); > + } > } > virBufferAddLit(buf, "</vlan>\n"); > return 0; > diff --git a/src/util/virnetdevopenvswitch.c > b/src/util/virnetdevopenvswitch.c > index 4fe077a..69cc1ac 100644 > --- a/src/util/virnetdevopenvswitch.c > +++ b/src/util/virnetdevopenvswitch.c > @@ -19,6 +19,7 @@ > * Dan Wendlandt <dan(a)nicira.com&gt; > * Kyle Mestery <kmestery(a)cisco.com&gt; > * Ansis Atteka <aatteka(a)nicira.com&gt; > + * James Robson <jrobson(a)websense.com&gt; The author list in our files usually only contains the original authors of the file and those who have contributed a "substantial" portion after creation.

> */ > > #include <config.h> > @@ -108,8 +109,13 @@ int virNetDevOpenvswitchAddPort(const char *brname, > const char *ifname, > virCommandAddArgList(cmd, "--timeout=5", "--", "--may-exist", > "add-port", > brname, ifname, NULL); > > - if (virBufferUse(&buf) != 0) > + if (virBufferUse(&buf) != 0) { > + if (virtVlan->nativeMode > 0) { > + virCommandAddArgFormat(cmd, "vlan_mode=%s", > virtVlan->nativeMode == 1 ? "native-tagged" : "native-untagged"); > + virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag); > + } > virCommandAddArgList(cmd, virBufferCurrentContent(&buf), NULL); > + } > > if (ovsport->profileID[0] == '\0') { > virCommandAddArgList(cmd, > diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c > index 2fe2017..b81c037 100644 > --- a/src/util/virnetdevvlan.c > +++ b/src/util/virnetdevvlan.c > @@ -17,6 +17,7 @@ > * > * Authors: > * Laine Stump <laine(a)redhat.com&gt; > + * James Robson <jrobson(a)websense.com&gt; > */ > > #include <config.h> > @@ -33,6 +34,8 @@ virNetDevVlanClear(virNetDevVlanPtr vlan) > { > VIR_FREE(vlan->tag); > vlan->nTags = 0; > + vlan->nativeMode = 0; > + vlan->nativeTag = 0; > } > > void > @@ -54,7 +57,9 @@ virNetDevVlanEqual(const virNetDevVlanPtr a, const > virNetDevVlanPtr b) > return false; > > if (a->trunk != b->trunk || > - a->nTags != b->nTags) { > + a->nTags != b->nTags || > + a->nativeMode != b->nativeMode || > + a->nativeTag != b->nativeTag) { > return false; > } > > @@ -89,6 +94,8 @@ virNetDevVlanCopy(virNetDevVlanPtr dst, const > virNetDevVlanPtr src) > > dst->trunk = src->trunk; > dst->nTags = src->nTags; > + dst->nativeMode = src->nativeMode; > + dst->nativeTag = src->nativeTag; > memcpy(dst->tag, src->tag, src->nTags * sizeof(*src->tag)); > return 0; > } > diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h > index c6b16ef..93426cc 100644 > --- a/src/util/virnetdevvlan.h > +++ b/src/util/virnetdevvlan.h > @@ -17,6 +17,7 @@ > * > * Authors: > * Laine Stump <laine(a)redhat.com&gt; > + * James Robson <jrobson(a)websense.com&gt; > */ > > #ifndef __VIR_NETDEV_VLAN_H__ > @@ -28,6 +29,8 @@ struct _virNetDevVlan { > bool trunk; /* true if this is a trunk */ > int nTags; /* number of tags in array */ > unsigned int *tag; /* pointer to array of tags */ > + short int nativeMode; /* 0=off, 1=tagged, 2=untagged */ > + unsigned int nativeTag; > }; > > void virNetDevVlanClear(virNetDevVlanPtr vlan); > diff --git a/tests/networkxml2xmlin/openvswitch-net.xml > b/tests/networkxml2xmlin/openvswitch-net.xml > index a3d82b1..2f6084d 100644 > --- a/tests/networkxml2xmlin/openvswitch-net.xml > +++ b/tests/networkxml2xmlin/openvswitch-net.xml > @@ -21,4 +21,13 @@ > <parameters profileid='alice-profile'/> > </virtualport> > </portgroup> > + <portgroup name='native'> > + <vlan trunk='yes'> > + <tag id='123' nativeMode='tagged'/> > + <tag id='444'/> > + </vlan> > + <virtualport> > + <parameters profileid='native-profile'/> > + </virtualport> > + </portgroup> > </network> > diff --git a/tests/networkxml2xmlout/openvswitch-net.xml > b/tests/networkxml2xmlout/openvswitch-net.xml > index a3d82b1..2f6084d 100644 > --- a/tests/networkxml2xmlout/openvswitch-net.xml > +++ b/tests/networkxml2xmlout/openvswitch-net.xml > @@ -21,4 +21,13 @@ > <parameters profileid='alice-profile'/> > </virtualport> > </portgroup> > + <portgroup name='native'> > + <vlan trunk='yes'> > + <tag id='123' nativeMode='tagged'/> > + <tag id='444'/> > + </vlan> > + <virtualport> > + <parameters profileid='native-profile'/> > + </virtualport> > + </portgroup> > </network> > > > Those are the only issues I see with this patch now. Kyle - what is your opinion of implementing the config this way. Does it seem workable? Or was my ignorance of the subject matter showing when I suggested putting "nativeMode='tagged'" as an attribute of a tag (rather than putting "nativeTag" and "nativeMode" attributes directly in <vlan>, as James did in the first version of the patch)? To report this as spam, please forward to spam(a)websense.com. Thank you.