[libvirt] [PATCHv3] Configure native vlan modes on Open vSwitch ports
james robson
jrobson at websense.com
Tue May 14 12:34:34 UTC 2013
On Fri, 2013-05-10 at 14:10 -0400, Laine Stump wrote:
> 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
> > <parameters
> > interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/>
> > </virtualport>
> > </interface>
> > + <interface type='bridge'>
> > + <b><vlan trunk='yes'></b>
> > + <b><tag id='42'/></b>
> > + <b><tag id='123' nativeMode='untagged'/></b>
> > + <b></vlan></b>
> > + ...
> > + </interface>
> > <devices>
> > ...</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><tag></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 @@
> > <parameters
> > interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/>
> > </virtualport>
> > </interface>
> > + <interface type='bridge'>
> > + <b><vlan trunk='yes'></b>
> > + <b><tag id='42'/></b>
> > + <b><tag id='123' nativeMode='untagged'/></b>
> > + <b></vlan></b>
> > + ...
> > + </interface>
> > <devices>
> > ...</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><tag></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><vlan></code> elements can also be specified in
> > a <code><portgroup></code> element, as well as directly in
> > a domain's <code><interface></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 at redhat.com>
> > + * James Robson <jrobson at websense.com>
> > */
> >
> > #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.
>
Ok, I can look at cleaning that up.
>
> > + 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 at nicira.com>
> > * Kyle Mestery <kmestery at cisco.com>
> > * Ansis Atteka <aatteka at nicira.com>
> > + * James Robson <jrobson at websense.com>
>
> 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.
Is there any guidance on what qualifies as "substantial"? I accept that
the change to util/virnetdevopenvswitch.c was very small, as is the
change to util/virnetdevvlan.h. What about the other files?
>
> > */
> >
> > #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 at redhat.com>
> > + * James Robson <jrobson at websense.com>
> > */
> >
> > #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 at redhat.com>
> > + * James Robson <jrobson at websense.com>
> > */
> >
> > #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 at websense.com. Thank you.
Protected by Websense Hosted Email Security -- www.websense.com
More information about the libvir-list
mailing list