[libvirt] [PATCHv2 0/4] support <vlan> element for interfaces and networks
danpb raised a (valid) objection about the XML format in the first version of these patches, so I've redone it to be like this: <vlan trunk='yes'> <tag id='42'/> <tag id='30'/> </vlan> The downside is that for a single vlan tag, it's more verbose: <vlan> <tag id='42'/> </vlan> On the other hand, it's more consistent. The rest of the patches in the series were unchanged (and even the data definition is unchanged). === The purpose of these patches is to define a data type that can properly describe an interface's vlan configuration (including multiple vlans for trunking), provide XML parser and formatter to translate to and from the new data type, add the new <vlan> element to both domain <interface> and to <network> in appropriate places, and to provide the new vlan data to the hypervisor driver. Once these 4 patches are applied, all that will be left to support vlan configuration for any network connection that supports it, is to request the vlan data for the interface with virDomainNetGetActualVlan(netdef). If that function returns NULL, there is no vlan config for this interface; if it's non-zero, it is a pointer to virNetDevVlan, and you can grab the data from there. Since I currently know of only two types of network connection we will be able to easily support this for (sr-iov VFs using PCI passthrough, and openvswitch), any type other than those will refuse to start if a vlan has been requested (see PATCH 4/4). This covers validation for the qemu and lxc drivers (since they use common code), but I'm not sure about xen, esx, or any other driver - they will likely require extra code to either implement transparent vlan configuration, or log an error if it's requested (my guess is that, in spite of our best efforts, *many* people will misunderstand the limitations of this functionality, and try to put all sorts of interfaces on vlans; it will be much friendlier to inform them of the problem rather than simply ignore the vlan config.
<portgroup> allows a <bandwidth> element, but the schema didn't have this. Since this makes for multiple elements in portgroup, they must be interleaved. <interface type='bridge'> needs to allow <virtualport> elements for openvswitch, but the schema didn't allow this. --- docs/schemas/domaincommon.rng | 3 +++ docs/schemas/network.rng | 11 ++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c85d763..238e57e 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1409,6 +1409,9 @@ <empty/> </element> </optional> + <optional> + <ref name="virtualPortProfile"/> + </optional> <ref name="interface-options"/> </interleave> </group> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 2ae879e..30c5a31 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -123,9 +123,14 @@ </choice> </attribute> </optional> - <optional> - <ref name="virtualPortProfile"/> - </optional> + <interleave> + <optional> + <ref name="virtualPortProfile"/> + </optional> + <optional> + <ref name="bandwidth"/> + </optional> + </interleave> </element> </zeroOrMore> -- 1.7.11.2
On 08/14/2012 01:15 AM, Laine Stump wrote:
<portgroup> allows a <bandwidth> element, but the schema didn't have this. Since this makes for multiple elements in portgroup, they must be interleaved.
<interface type='bridge'> needs to allow <virtualport> elements for openvswitch, but the schema didn't allow this. --- docs/schemas/domaincommon.rng | 3 +++ docs/schemas/network.rng | 11 ++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
To allow for the possibility of vlan "trunks", which have more than one vlan tag associated with them, we need a vlan struct. Since it will be used by multiple files in src/util, src/conf, src/network, and src/qemu, it must be defined in src/util. Unfortunately there isn't currently a common file for simple netdev data definitions, so I created a new file. --- src/Makefile.am | 1 + src/libvirt_private.syms | 6 ++++ src/util/virnetdevvlan.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevvlan.h | 38 +++++++++++++++++++++ 4 files changed, 133 insertions(+) create mode 100644 src/util/virnetdevvlan.c create mode 100644 src/util/virnetdevvlan.h diff --git a/src/Makefile.am b/src/Makefile.am index 8a93a32..840bb82 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -104,6 +104,7 @@ UTIL_SOURCES = \ util/virnetdevopenvswitch.h util/virnetdevopenvswitch.c \ util/virnetdevtap.h util/virnetdevtap.c \ util/virnetdevveth.h util/virnetdevveth.c \ + util/virnetdevvlan.h util/virnetdevvlan.c \ util/virnetdevvportprofile.h util/virnetdevvportprofile.c \ util/virnetlink.c util/virnetlink.h \ util/virrandom.h util/virrandom.c \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 89fb1f4..d20f9e3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1425,6 +1425,12 @@ virNetDevVethCreate; virNetDevVethDelete; +# virnetdevvlan.h +virNetDevVlanClear; +virNetDevVlanCopy; +virNetDevVlanEqual; +virNetDevVlanFree; + # virnetdevvportprofile.h virNetDevVPortProfileAssociate; virNetDevVPortProfileCheckComplete; diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c new file mode 100644 index 0000000..d66902f --- /dev/null +++ b/src/util/virnetdevvlan.c @@ -0,0 +1,88 @@ +/* + * Copyright (C) 2009-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Laine Stump <laine@redhat.com> + */ + +#include <config.h> + +#include "internal.h" +#include "virterror_internal.h" +#include "virnetdevvlan.h" +#include "memory.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +void +virNetDevVlanClear(virNetDevVlanPtr vlan) +{ + VIR_FREE(vlan->tag); + vlan->nTags = 0; +} + +void +virNetDevVlanFree(virNetDevVlanPtr vlan) +{ + if (vlan) + virNetDevVlanClear(vlan); + VIR_FREE(vlan); +} + +int +virNetDevVlanEqual(const virNetDevVlanPtr a, const virNetDevVlanPtr b) +{ + int ii; + + if (!(a || b)) + return true; + if (!a || !b) + return false; + + if (a->trunk != b->trunk || + a->nTags != b->nTags) { + return false; + } + + for (ii = 0; ii < a->nTags; ii++) { + if (a->tag[ii] != b->tag[ii]) + return false; + } + return true; +} + +/* + * virNetDevVlanCopy - copy from src into (already existing) dst. + * If src is NULL, dst will have nTags set to 0. + * dst is assumed to be empty on entry. + */ +int +virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlanPtr src) +{ + if (!src || src->nTags == 0) + return 0; + + if (VIR_ALLOC_N(dst->tag, src->nTags) < 0) { + virReportOOMError(); + return -1; + } + + dst->trunk = src->trunk; + dst->nTags = src->nTags; + memmove(dst->tag, src->tag, src->nTags * sizeof(*src->tag)); + return 0; +} diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h new file mode 100644 index 0000000..7a046f9 --- /dev/null +++ b/src/util/virnetdevvlan.h @@ -0,0 +1,38 @@ +/* + * Copyright (C) 2009-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Laine Stump <laine@redhat.com> + */ + +#ifndef __VIR_NETDEV_VLAN_H__ +# define __VIR_NETDEV_VLAN_H__ + +typedef struct _virNetDevVlan virNetDevVlan; +typedef virNetDevVlan *virNetDevVlanPtr; +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 */ +}; + +void virNetDevVlanClear(virNetDevVlanPtr vlan); +void virNetDevVlanFree(virNetDevVlanPtr vlan); +int virNetDevVlanEqual(const virNetDevVlanPtr a, const virNetDevVlanPtr b); +int virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlanPtr src); + +#endif /* __VIR_NETDEV_VLAN_H__ */ -- 1.7.11.2
On 08/14/2012 01:15 AM, Laine Stump wrote:
To allow for the possibility of vlan "trunks", which have more than one vlan tag associated with them, we need a vlan struct. Since it will be used by multiple files in src/util, src/conf, src/network, and src/qemu, it must be defined in src/util. Unfortunately there isn't currently a common file for simple netdev data definitions, so I created a new file.
+int +virNetDevVlanEqual(const virNetDevVlanPtr a, const virNetDevVlanPtr b) +{ + int ii; + + if (!(a || b)) + return true; + if (!a || !b) + return false; + + if (a->trunk != b->trunk || + a->nTags != b->nTags) { + return false; + } + + for (ii = 0; ii < a->nTags; ii++) { + if (a->tag[ii] != b->tag[ii]) + return false;
I think this requires the user to list tags in identical order. Should two virNetDevVlanPtr be considered equal if the only difference is the order in which tags are listed, or is there some other reason why an unsorted list would lose significance if sorted?
+int +virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlanPtr src) +{ + if (!src || src->nTags == 0) + return 0; + + if (VIR_ALLOC_N(dst->tag, src->nTags) < 0) { + virReportOOMError(); + return -1; + } + + dst->trunk = src->trunk; + dst->nTags = src->nTags; + memmove(dst->tag, src->tag, src->nTags * sizeof(*src->tag));
src and dst better not overlap; therefore, memcpy() is a better fit. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/14/2012 06:55 PM, Eric Blake wrote:
On 08/14/2012 01:15 AM, Laine Stump wrote:
To allow for the possibility of vlan "trunks", which have more than one vlan tag associated with them, we need a vlan struct. Since it will be used by multiple files in src/util, src/conf, src/network, and src/qemu, it must be defined in src/util. Unfortunately there isn't currently a common file for simple netdev data definitions, so I created a new file. +int +virNetDevVlanEqual(const virNetDevVlanPtr a, const virNetDevVlanPtr b) +{ + int ii; + + if (!(a || b)) + return true; + if (!a || !b) + return false; + + if (a->trunk != b->trunk || + a->nTags != b->nTags) { + return false; + } + + for (ii = 0; ii < a->nTags; ii++) { + if (a->tag[ii] != b->tag[ii]) + return false; I think this requires the user to list tags in identical order. Should two virNetDevVlanPtr be considered equal if the only difference is the order in which tags are listed, or is there some other reason why an unsorted list would lose significance if sorted?
Really it was just lack of deeper thought, and the fact that my only planned used for this function is to decide if things have changed enough to, for example, disallow a "live" change of network config (currently it's not used at all). I've changed the algorithm to the following. Unfortunately I again wasn't thinking, and changed it during a git rebase, so I don't have a diff handy. Should I resend the entire patch, or is seeing this changed function enough for an ACK? (Note that the following would return true if one array was {1,1,2,2,2} and the other was {2,1,2,1,1} (i.e. if both have all the same members, but with differing counts of each). However, it makes no sense for a vlan trunk to list the same tag more than once anyway, so I guess effectively they *would* be the same.) int virNetDevVlanEqual(const virNetDevVlanPtr a, const virNetDevVlanPtr b) { int ai, bi; if (!(a || b)) return true; if (!a || !b) return false; if (a->trunk != b->trunk || a->nTags != b->nTags) { return false; } for (ai = 0; ai < a->nTags; ai++) { for (bi = 0; bi < b->nTags; bi++) { if (a->tag[ai] == b->tag[bi]) break; } if (bi >= b->nTags) { /* no matches for a->tag[ai] anywhere in b->tag */ return false; } } return true; }
+int +virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlanPtr src) +{ + if (!src || src->nTags == 0) + return 0; + + if (VIR_ALLOC_N(dst->tag, src->nTags) < 0) { + virReportOOMError(); + return -1; + } + + dst->trunk = src->trunk; + dst->nTags = src->nTags; + memmove(dst->tag, src->tag, src->nTags * sizeof(*src->tag)); src and dst better not overlap; therefore, memcpy() is a better fit
Okay, changed that.
On 08/14/2012 11:01 PM, Laine Stump wrote:
I've changed the algorithm to the following. Unfortunately I again wasn't thinking, and changed it during a git rebase, so I don't have a diff handy. Should I resend the entire patch, or is seeing this changed function enough for an ACK?
Nah, seeing this is fine.
(Note that the following would return true if one array was {1,1,2,2,2} and the other was {2,1,2,1,1} (i.e. if both have all the same members, but with differing counts of each). However, it makes no sense for a vlan trunk to list the same tag more than once anyway, so I guess effectively they *would* be the same.)
Yeah, that makes me wonder if we should do duplicate detection at XML parse time. But it's not the end of the world if we don't (the behavior is the same, and most people won't be tickling this corner case).
for (ai = 0; ai < a->nTags; ai++) { for (bi = 0; bi < b->nTags; bi++) { if (a->tag[ai] == b->tag[bi]) break; } if (bi >= b->nTags) { /* no matches for a->tag[ai] anywhere in b->tag */ return false; } }
That's O(n^2). As long as N is small, it doesn't matter; and considering that N cannot be larger than 4095 and most people won't bundle that many vlans together on a trunk, I think we're fine. In other words, any rewrite to use conversions to two bitsets followed by an intersection to cut it down to O(n) is probably premature optimization not worth doing. ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The following config elements now support a <vlan> subelements: within a domain: <interface>, and the <actual> subelement of <interface> within a network: the toplevel, as well as any <portgroup> Each vlan element must have one or more <tag id='n'/> subelements. If there is more than one tag, it is assumed that vlan trunking is being requested. If trunking is required with only a single tag, the attribute "trunk='yes'" should be added to the toplevel <vlan> element. Some examples: <interface type='hostdev'/> <vlan> <tag id='42'/> </vlan> <mac address='52:54:00:12:34:56'/> ... </interface> <network> <name>vlan-net</name> <vlan trunk='yes'> <tag id='30'/> </vlan> <virtualport type='openvswitch'/> </network> <interface type='network'/> <source network='vlan-net'/> ... </interface> <network> <name>trunk-vlan</name> <vlan> <tag id='42'/> <tag id='43'/> </vlan> ... </network> <network> <name>multi</name> ... <portgroup name='production'/> <vlan> <tag id='42'/> </vlan> </portgroup> <portgroup name='test'/> <vlan> <tag id='666'/> </vlan> </portgroup> </network> <interface type='network'/> <source network='multi' portgroup='test'/> ... </interface> IMPORTANT NOTE: As of this patch there is no backend support for the vlan element for *any* network device type. When support is added in later patches, it will only be for those select network types that support setting up a vlan on the host side, without the guest's involvement. (For example, it will be possible to configure a vlan for a guest connected to an openvswitch bridge, but it won't be possible to do that for one that is connected to a standard Linux host bridge.) --- docs/formatdomain.html.in | 40 +++++++ docs/formatnetwork.html.in | 50 +++++++++ docs/schemas/domaincommon.rng | 3 + docs/schemas/network.rng | 6 + docs/schemas/networkcommon.rng | 19 ++++ po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/domain_conf.c | 31 ++++- src/conf/domain_conf.h | 4 + src/conf/netdev_vlan_conf.c | 125 +++++++++++++++++++++ src/conf/netdev_vlan_conf.h | 33 ++++++ src/conf/network_conf.c | 18 ++- src/conf/network_conf.h | 3 + src/libvirt_private.syms | 6 + tests/networkxml2xmlin/8021Qbh-net.xml | 3 + tests/networkxml2xmlin/openvswitch-net.xml | 8 ++ tests/networkxml2xmlout/8021Qbh-net.xml | 3 + tests/networkxml2xmlout/openvswitch-net.xml | 8 ++ .../qemuxml2argvdata/qemuxml2argv-net-hostdev.xml | 3 + .../qemuxml2argv-net-openvswitch.xml | 38 +++++++ .../qemuxml2argv-net-virtio-network-portgroup.xml | 3 + tests/qemuxml2xmltest.c | 1 + 22 files changed, 404 insertions(+), 5 deletions(-) create mode 100644 src/conf/netdev_vlan_conf.c create mode 100644 src/conf/netdev_vlan_conf.h create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f3b3fa8..a81100c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2855,6 +2855,46 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">Since 0.9.4</span> </p> + <h5><a name="elementVlanTag">Setting VLAN tag (on supported network types only)</a></h5> + +<pre> + ... + <devices> + <interface type='bridge'> + <b><vlan></b> + <b><tag id='42'/></b> + <b></vlan></b> + <source bridge='ovsbr0'/> + <virtualport type='openvswitch'> + <parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> + </virtualport> + </interface> + <devices> + ...</pre> + + <p> + If (and only if) the network connection used by the guest + supports vlan tagging transparent to the guest, an + optional <code><vlan></code> element can specify one or + more vlan tags to apply to the guest's network + traffic <span class="since">Since 0.10.0</span>. (openvswitch + and type='hostdev' SR-IOV interfaces do support transparent vlan + tagging of guest traffic; everything else, including standard + linux bridges and libvirt's own virtual networks, <b>do not</b> + support it. 802.1Qbh (vn-link) and 802.1Qbg (VEPA) switches + provide their own way (outside of libvirt) to tag guest traffic + onto specific vlans.) To allow for specification of multiple + tags (in the case of vlan trunking), a + subelement, <code><tag%gt;</code>, specifies which vlan tag + to use (for example: <code><tag id='42'/></code>. If an + interface has more than one <code><vlan></code> element + defined, it is assumed that the user wants to do VLAN trunking + using all the specified tags. In the case that vlan trunking + with a single tag is desired, the optional + attribute <code>trunk='yes'</code> can be added to the toplevel + vlan element. + </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 0ba4d2d..612c9de 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -290,6 +290,56 @@ <span class="since">Since 0.9.4</span> </p> + <h5><a name="elementVlanTag">Setting VLAN tag (on supported network types only)</a></h5> + +<pre> + ... + <devices> + <interface type='bridge'> + <b><vlan trunk='yes'></b> + <b><tag id='42'/></b> + <b><tag id='47'/></b> + <b></vlan></b> + <source bridge='ovsbr0'/> + <virtualport type='openvswitch'> + <parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> + </virtualport> + </interface> + <devices> + ...</pre> + + <p> + If (and only if) the network type supports vlan tagging + transparent to the guest, an optional <code><vlan></code> + element can specify one or more vlan tags to apply to the + traffic of all guests using this + network <span class="since">Since 0.10.0</span>. (openvswitch + and type='hostdev' SR-IOV networks do support transparent vlan + tagging of guest traffic; everything else, including standard + linux bridges and libvirt's own virtual networks, <b>do not</b> + support it. 802.1Qbh (vn-link) and 802.1Qbg (VEPA) switches + provide their own way (outside of libvirt) to tag guest traffic + onto specific vlans.) As expected, the <code>tag</code> + attribute specifies which vlan tag to use. If a network has more + than one <code><vlan></code> element defined, it is + assumed that the user wants to do VLAN trunking using all the + specified tags. In the case that vlan trunking with a single tag + is desired, the optional attribute <code>trunk='yes'</code> can + be added to the vlan element. + </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 + that a vlan tag is specified in multiple locations, the setting + in <code><interface></code> takes precedence, followed by + the setting in the <code><portgroup></code> selected by + the interface config. The <code><vlan></code> + in <code><network></code> will be selected only if none is + given in <code><portgroup></code> + or <code><interface></code>. + </p> + <h5><a name="elementsPortgroup">Portgroups</a></h5> <pre> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 238e57e..6f333e9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1712,6 +1712,9 @@ <optional> <ref name="bandwidth"/> </optional> + <optional> + <ref name="vlan"/> + </optional> </interleave> </define> <!-- diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 30c5a31..eac599c 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -130,6 +130,9 @@ <optional> <ref name="bandwidth"/> </optional> + <optional> + <ref name="vlan"/> + </optional> </interleave> </element> </zeroOrMore> @@ -178,6 +181,9 @@ <ref name="bandwidth"/> </optional> <optional> + <ref name="vlan"/> + </optional> + <optional> <element name="link"> <attribute name="state"> <choice> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index f2c3330..1700c22 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -184,4 +184,23 @@ <param name="pattern">(ipv4)|(ipv6)</param> </data> </define> + + <define name="vlan"> + <element name="vlan"> + <optional> + <attribute name="trunk"> + <value>yes</value> + </attribute> + </optional> + <oneOrMore> + <element name="tag"> + <attribute name="id"> + <data type="unsignedInt"> + <param name="maxInclusive">4095</param> + </data> + </attribute> + </element> + </oneOrMore> + </element> + </define> </grammar> diff --git a/po/POTFILES.in b/po/POTFILES.in index e617952..6424726 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -11,6 +11,7 @@ src/conf/domain_conf.c src/conf/domain_event.c src/conf/interface_conf.c src/conf/netdev_bandwidth_conf.c +src/conf/netdev_vlan_conf.c src/conf/netdev_vport_profile_conf.c src/conf/network_conf.c src/conf/node_device_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index 840bb82..979ce2f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -143,7 +143,8 @@ LOCK_DRIVER_SANLOCK_SOURCES = \ NETDEV_CONF_SOURCES = \ conf/netdev_bandwidth_conf.h conf/netdev_bandwidth_conf.c \ - conf/netdev_vport_profile_conf.h conf/netdev_vport_profile_conf.c + conf/netdev_vport_profile_conf.h conf/netdev_vport_profile_conf.c \ + conf/netdev_vlan_conf.h conf/netdev_vlan_conf.c # XML configuration format handling sources # Domain driver generic impl APIs diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0cda374..ae95d81 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -51,6 +51,7 @@ #include "secret_conf.h" #include "netdev_vport_profile_conf.h" #include "netdev_bandwidth_conf.h" +#include "netdev_vlan_conf.h" #include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -1031,7 +1032,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def) VIR_FREE(def->virtPortProfile); virNetDevBandwidthFree(def->bandwidth); - + virNetDevVlanClear(&def->vlan); VIR_FREE(def); } @@ -1092,6 +1093,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def) virNWFilterHashTableFree(def->filterparams); virNetDevBandwidthFree(def->bandwidth); + virNetDevVlanClear(&def->vlan); VIR_FREE(def); } @@ -4366,6 +4368,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, int ret = -1; xmlNodePtr save_ctxt = ctxt->node; xmlNodePtr bandwidth_node = NULL; + xmlNodePtr vlanNode; xmlNodePtr virtPortNode; char *type = NULL; char *mode = NULL; @@ -4463,6 +4466,10 @@ virDomainActualNetDefParseXML(xmlNodePtr node, !(actual->bandwidth = virNetDevBandwidthParse(bandwidth_node))) goto error; + vlanNode = virXPathNode("./vlan", ctxt); + if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &actual->vlan) < 0) + goto error; + *def = actual; actual = NULL; ret = 0; @@ -4640,6 +4647,9 @@ virDomainNetDefParseXML(virCapsPtr caps, } else if (xmlStrEqual(cur->name, BAD_CAST "bandwidth")) { if (!(def->bandwidth = virNetDevBandwidthParse(cur))) goto error; + } else if (xmlStrEqual(cur->name, BAD_CAST "vlan")) { + if (virNetDevVlanParse(cur, ctxt, &def->vlan) < 0) + goto error; } } cur = cur->next; @@ -11618,8 +11628,10 @@ virDomainActualNetDefFormat(virBufferPtr buf, return -1; } + if (virNetDevVlanFormat(&def->vlan, buf) < 0) + return -1; if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) - return -1; + return -1; if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) return -1; @@ -11720,8 +11732,10 @@ virDomainNetDefFormat(virBufferPtr buf, break; } + if (virNetDevVlanFormat(&def->vlan, buf) < 0) + return -1; if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) - return -1; + return -1; virBufferEscapeString(buf, "<script path='%s'/>\n", def->script); if (def->ifname && @@ -15078,6 +15092,17 @@ virDomainNetGetActualBandwidth(virDomainNetDefPtr iface) return iface->bandwidth; } +virNetDevVlanPtr +virDomainNetGetActualVlan(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && + iface->data.network.actual && + iface->data.network.actual->vlan.nTags > 0) + return &iface->data.network.actual->vlan; + if (iface->vlan.nTags > 0) + return &iface->vlan; + return 0; +} /* Return listens[ii] from the appropriate union for the graphics * type, or NULL if this is an unsuitable type, or the index is out of diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f36f7d6..eea81d5 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -43,6 +43,7 @@ # include "virnetdevvportprofile.h" # include "virnetdevopenvswitch.h" # include "virnetdevbandwidth.h" +# include "virnetdevvlan.h" # include "virobject.h" /* forward declarations of all device types, required by @@ -781,6 +782,7 @@ struct _virDomainActualNetDef { } data; virNetDevVPortProfilePtr virtPortProfile; virNetDevBandwidthPtr bandwidth; + virNetDevVlan vlan; }; /* Stores the virtual network interface configuration */ @@ -845,6 +847,7 @@ struct _virDomainNetDef { char *filter; virNWFilterHashTablePtr filterparams; virNetDevBandwidthPtr bandwidth; + virNetDevVlan vlan; int linkstate; }; @@ -2039,6 +2042,7 @@ virNetDevVPortProfilePtr virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface); virNetDevBandwidthPtr virDomainNetGetActualBandwidth(virDomainNetDefPtr iface); +virNetDevVlanPtr virDomainNetGetActualVlan(virDomainNetDefPtr iface); int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller); diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c new file mode 100644 index 0000000..66adb17 --- /dev/null +++ b/src/conf/netdev_vlan_conf.c @@ -0,0 +1,125 @@ +/* + * Copyright (C) 2009-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Laine Stump <laine@redhat.com> + */ + +#include <config.h> + +#include "netdev_vlan_conf.h" +#include "virterror_internal.h" +#include "memory.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +int +virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr def) +{ + int ret = -1; + xmlNodePtr save = ctxt->node; + const char *trunk; + xmlNodePtr *tagNodes = NULL; + int nTags; + + ctxt->node = node; + + nTags = virXPathNodeSet("./tag", ctxt, &tagNodes); + if (nTags < 0) + goto error; + + if (nTags == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing tag id - each <vlan> must have " + "at least one <tag id='n'/> subelement")); + goto error; + } + + if (nTags > 0) { + int ii; + + if (VIR_ALLOC_N(def->tag, nTags) < 0) { + virReportOOMError(); + goto error; + } + + for (ii = 0; ii < nTags; ii++) { + unsigned long id; + + ctxt->node = tagNodes[ii]; + if (virXPathULong("string(./@id)", ctxt, &id) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing or invalid vlan tag id attribute")); + goto error; + } + if (id > 4095) { + virReportError(VIR_ERR_XML_ERROR, + _("vlan tag id %lu too large (maximum 4095)"), id); + goto error; + } + def->tag[ii] = id; + } + } + def->nTags = nTags; + + /* now that we know how many tags there are, look for an explicit + * trunk setting. + */ + if (nTags > 1) + def->trunk = true; + + ctxt->node = node; + if ((trunk = virXPathString("string(./@trunk)", ctxt)) != NULL) { + def->trunk = STRCASEEQ(trunk, "yes"); + if (nTags > 1 && !def->trunk) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid \"trunk='%s'\" in <vlan> - trunk='yes' " + "is required for more than one vlan tag"), trunk); + goto error; + } + } + + ret = 0; +error: + ctxt->node = save; + VIR_FREE(tagNodes); + if (ret < 0) + virNetDevVlanClear(def); + return ret; +} + +int +virNetDevVlanFormat(virNetDevVlanPtr def, virBufferPtr buf) +{ + int ii; + + if (def->nTags == 0) + return 0; + + if (!def->tag) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing vlan tag data")); + return -1; + } + + 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]); + } + virBufferAddLit(buf, "</vlan>\n"); + return 0; +} diff --git a/src/conf/netdev_vlan_conf.h b/src/conf/netdev_vlan_conf.h new file mode 100644 index 0000000..1453dcd --- /dev/null +++ b/src/conf/netdev_vlan_conf.h @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2009-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Laine Stump <laine@redhat.com> + */ + +#ifndef __VIR_NETDEV_VLAN_CONF_H__ +# define __VIR_NETDEV_VLAN_CONF_H__ + +# include "internal.h" +# include "virnetdevvlan.h" +# include "buf.h" +# include "xml.h" + +int virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr def); +int virNetDevVlanFormat(virNetDevVlanPtr def, virBufferPtr buf); + +#endif /* __VIR_NETDEV_VPORT_PROFILE_CONF_H__ */ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 45f8e69..db8c62f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -36,6 +36,7 @@ #include "network_conf.h" #include "netdev_vport_profile_conf.h" #include "netdev_bandwidth_conf.h" +#include "netdev_vlan_conf.h" #include "memory.h" #include "xml.h" #include "uuid.h" @@ -88,6 +89,7 @@ virPortGroupDefClear(virPortGroupDefPtr def) VIR_FREE(def->name); VIR_FREE(def->virtPortProfile); virNetDevBandwidthFree(def->bandwidth); + virNetDevVlanClear(&def->vlan); def->bandwidth = NULL; } @@ -187,7 +189,7 @@ void virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def->virtPortProfile); virNetDevBandwidthFree(def->bandwidth); - + virNetDevVlanClear(&def->vlan); VIR_FREE(def); } @@ -890,6 +892,7 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, xmlNodePtr save; xmlNodePtr virtPortNode; + xmlNodePtr vlanNode; xmlNodePtr bandwidth_node; char *isDefault = NULL; @@ -915,6 +918,10 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, goto error; } + vlanNode = virXPathNode("./vlan", ctxt); + if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0) + goto error; + result = 0; error: if (result < 0) { @@ -943,6 +950,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) char *forwardDev = NULL; xmlNodePtr save = ctxt->node; xmlNodePtr bandwidthNode = NULL; + xmlNodePtr vlanNode; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -982,6 +990,10 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) (def->bandwidth = virNetDevBandwidthParse(bandwidthNode)) == NULL) goto error; + vlanNode = virXPathNode("./vlan", ctxt); + if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0) + goto error; + /* Parse bridge information */ def->bridge = virXPathString("string(./bridge[1]/@name)", ctxt); stp = virXPathString("string(./bridge[1]/@stp)", ctxt); @@ -1448,6 +1460,8 @@ virPortGroupDefFormat(virBufferPtr buf, } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 4); + if (virNetDevVlanFormat(&def->vlan, buf) < 0) + return -1; if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) return -1; virNetDevBandwidthFormat(def->bandwidth, buf); @@ -1542,6 +1556,8 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) goto error; virBufferAdjustIndent(&buf, 2); + if (virNetDevVlanFormat(&def->vlan, &buf) < 0) + goto error; if (virNetDevBandwidthFormat(def->bandwidth, &buf) < 0) goto error; virBufferAdjustIndent(&buf, -2); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 32fc765..a029f70 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -35,6 +35,7 @@ # include "virsocketaddr.h" # include "virnetdevbandwidth.h" # include "virnetdevvportprofile.h" +# include "virnetdevvlan.h" # include "virmacaddr.h" enum virNetworkForwardType { @@ -148,6 +149,7 @@ struct _virPortGroupDef { bool isDefault; virNetDevVPortProfilePtr virtPortProfile; virNetDevBandwidthPtr bandwidth; + virNetDevVlan vlan; }; typedef struct _virNetworkDef virNetworkDef; @@ -185,6 +187,7 @@ struct _virNetworkDef { size_t nPortGroups; virPortGroupDefPtr portGroups; virNetDevBandwidthPtr bandwidth; + virNetDevVlan vlan; }; typedef struct _virNetworkObj virNetworkObj; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d20f9e3..689377e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -406,6 +406,7 @@ virDomainNetGetActualDirectMode; virDomainNetGetActualHostdev; virDomainNetGetActualType; virDomainNetGetActualVirtPortProfile; +virDomainNetGetActualVlan; virDomainNetIndexByMac; virDomainNetInsert; virDomainNetRemove; @@ -787,6 +788,11 @@ virNetDevBandwidthFormat; virNetDevBandwidthParse; +#netdev_vlan_conf.h +virNetDevVlanFormat; +virNetDevVlanParse; + + # netdev_vportprofile_conf.h virNetDevVPortProfileFormat; virNetDevVPortProfileParse; diff --git a/tests/networkxml2xmlin/8021Qbh-net.xml b/tests/networkxml2xmlin/8021Qbh-net.xml index 2d779dc..3aaab61 100644 --- a/tests/networkxml2xmlin/8021Qbh-net.xml +++ b/tests/networkxml2xmlin/8021Qbh-net.xml @@ -8,6 +8,9 @@ <interface dev="eth4"/> <interface dev="eth5"/> </forward> + <vlan> + <tag id='549'/> + </vlan> <virtualport type="802.1Qbh"> <parameters profileid="spongebob24"/> </virtualport> diff --git a/tests/networkxml2xmlin/openvswitch-net.xml b/tests/networkxml2xmlin/openvswitch-net.xml index 8aa1897..a3d82b1 100644 --- a/tests/networkxml2xmlin/openvswitch-net.xml +++ b/tests/networkxml2xmlin/openvswitch-net.xml @@ -4,11 +4,19 @@ <forward mode='bridge'/> <virtualport type='openvswitch'/> <portgroup name='bob' default='yes'> + <vlan trunk='yes'> + <tag id='666'/> + </vlan> <virtualport> <parameters profileid='bob-profile'/> </virtualport> </portgroup> <portgroup name='alice'> + <vlan trunk='yes'> + <tag id='777'/> + <tag id='888'/> + <tag id='999'/> + </vlan> <virtualport> <parameters profileid='alice-profile'/> </virtualport> diff --git a/tests/networkxml2xmlout/8021Qbh-net.xml b/tests/networkxml2xmlout/8021Qbh-net.xml index d4d5b4b..281466a 100644 --- a/tests/networkxml2xmlout/8021Qbh-net.xml +++ b/tests/networkxml2xmlout/8021Qbh-net.xml @@ -8,6 +8,9 @@ <interface dev='eth4'/> <interface dev='eth5'/> </forward> + <vlan> + <tag id='549'/> + </vlan> <virtualport type='802.1Qbh'> <parameters profileid='spongebob24'/> </virtualport> diff --git a/tests/networkxml2xmlout/openvswitch-net.xml b/tests/networkxml2xmlout/openvswitch-net.xml index 8aa1897..a3d82b1 100644 --- a/tests/networkxml2xmlout/openvswitch-net.xml +++ b/tests/networkxml2xmlout/openvswitch-net.xml @@ -4,11 +4,19 @@ <forward mode='bridge'/> <virtualport type='openvswitch'/> <portgroup name='bob' default='yes'> + <vlan trunk='yes'> + <tag id='666'/> + </vlan> <virtualport> <parameters profileid='bob-profile'/> </virtualport> </portgroup> <portgroup name='alice'> + <vlan trunk='yes'> + <tag id='777'/> + <tag id='888'/> + <tag id='999'/> + </vlan> <virtualport> <parameters profileid='alice-profile'/> </virtualport> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml index 51b09e9..81f70d0 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml @@ -26,6 +26,9 @@ <source> <address type='pci' domain='0x0002' bus='0x03' slot='0x07' function='0x1'/> </source> + <vlan> + <tag id='42'/> + </vlan> <virtualport type='802.1Qbg'> <parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> </virtualport> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml new file mode 100644 index 0000000..ff09844 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml @@ -0,0 +1,38 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <interface type='network'> + <mac address='00:11:22:33:44:55'/> + <source network='ovs-net'/> + <vlan trunk='yes'> + <tag id='42'/> + <tag id='48'/> + <tag id='456'/> + </vlan> + <virtualport type='openvswitch'> + <parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f' profileid='bob'/> + </virtualport> + </interface> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml index 6d379a0..c84ed3f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml @@ -24,6 +24,9 @@ <interface type='network'> <mac address='00:11:22:33:44:55'/> <source network='rednet' portgroup='bob'/> + <vlan> + <tag id='4095'/> + </vlan> <virtualport type='802.1Qbg'> <parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> </virtualport> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index dcdba4f..da298b7 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -178,6 +178,7 @@ mymain(void) DO_TEST("net-eth-ifname"); DO_TEST("net-virtio-network-portgroup"); DO_TEST("net-hostdev"); + DO_TEST("net-openvswitch"); DO_TEST("sound"); DO_TEST("sound-device"); DO_TEST("net-bandwidth"); -- 1.7.11.2
On 08/14/2012 01:15 AM, Laine Stump wrote:
The following config elements now support a <vlan> subelements:
within a domain: <interface>, and the <actual> subelement of <interface> within a network: the toplevel, as well as any <portgroup>
Each vlan element must have one or more <tag id='n'/> subelements. If there is more than one tag, it is assumed that vlan trunking is being requested. If trunking is required with only a single tag, the attribute "trunk='yes'" should be added to the toplevel <vlan> element.
IMPORTANT NOTE: As of this patch there is no backend support for the vlan element for *any* network device type. When support is added in later patches, it will only be for those select network types that support setting up a vlan on the host side, without the guest's involvement. (For example, it will be possible to configure a vlan for a guest connected to an openvswitch bridge, but it won't be possible
+ <element name="tag"> + <attribute name="id"> + <data type="unsignedInt"> + <param name="maxInclusive">4095</param> + </data> + </attribute>
I would also add <empty/> here, since the user should always be using <tag/> rather than <tag><sub/></tag> or <tag>data</tag>.
+int +virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr def) +{ + int ret = -1; + xmlNodePtr save = ctxt->node; + const char *trunk; + xmlNodePtr *tagNodes = NULL; + int nTags; + + ctxt->node = node; + + nTags = virXPathNodeSet("./tag", ctxt, &tagNodes); + if (nTags < 0) + goto error; + + if (nTags == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing tag id - each <vlan> must have " + "at least one <tag id='n'/> subelement")); + goto error; + } + + if (nTags > 0) {
This 'if' is spurious, given the above two statements always short-circuiting to error. Cosmetic, though.
+ /* now that we know how many tags there are, look for an explicit + * trunk setting. + */ + if (nTags > 1) + def->trunk = true; + + ctxt->node = node; + if ((trunk = virXPathString("string(./@trunk)", ctxt)) != NULL) { + def->trunk = STRCASEEQ(trunk, "yes"); + if (nTags > 1 && !def->trunk) {
It took me a couple reads to see - this says the user can pass trunk='garbage' for a single <tag> (it won't get past the RNG validation, but our C code doesn't care), if they passed trunk at all, it MUST be trunk='yes' for multiple <tag>s. Okay. ACK with nits addressed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/14/2012 07:08 PM, Eric Blake wrote:
On 08/14/2012 01:15 AM, Laine Stump wrote:
The following config elements now support a <vlan> subelements:
within a domain: <interface>, and the <actual> subelement of <interface> within a network: the toplevel, as well as any <portgroup>
Each vlan element must have one or more <tag id='n'/> subelements. If there is more than one tag, it is assumed that vlan trunking is being requested. If trunking is required with only a single tag, the attribute "trunk='yes'" should be added to the toplevel <vlan> element.
IMPORTANT NOTE: As of this patch there is no backend support for the vlan element for *any* network device type. When support is added in later patches, it will only be for those select network types that support setting up a vlan on the host side, without the guest's involvement. (For example, it will be possible to configure a vlan for a guest connected to an openvswitch bridge, but it won't be possible + <element name="tag"> + <attribute name="id"> + <data type="unsignedInt"> + <param name="maxInclusive">4095</param> + </data> + </attribute> I would also add <empty/> here, since the user should always be using <tag/> rather than <tag><sub/></tag> or <tag>data</tag>.
Fixed.
+int +virNetDevVlanParse(xmlNodePtr node, xmlXPathContextPtr ctxt, virNetDevVlanPtr def) +{ + int ret = -1; + xmlNodePtr save = ctxt->node; + const char *trunk; + xmlNodePtr *tagNodes = NULL; + int nTags; + + ctxt->node = node; + + nTags = virXPathNodeSet("./tag", ctxt, &tagNodes); + if (nTags < 0) + goto error; + + if (nTags == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing tag id - each <vlan> must have " + "at least one <tag id='n'/> subelement")); + goto error; + } + + if (nTags > 0) { This 'if' is spurious, given the above two statements always short-circuiting to error. Cosmetic, though.
Removed.
+ /* now that we know how many tags there are, look for an explicit + * trunk setting. + */ + if (nTags > 1) + def->trunk = true; + + ctxt->node = node; + if ((trunk = virXPathString("string(./@trunk)", ctxt)) != NULL) { + def->trunk = STRCASEEQ(trunk, "yes"); + if (nTags > 1 && !def->trunk) { It took me a couple reads to see - this says the user can pass trunk='garbage' for a single <tag> (it won't get past the RNG validation, but our C code doesn't care), if they passed trunk at all, it MUST be trunk='yes' for multiple <tag>s. Okay.
Okay. I changed it to quietly ignore if someone puts "trunk='no'" when nTags == 1, but log an error otherwise (unless it's trunk='yes' of course - that's always okay.)
ACK with nits addressed.
I'll push it after I heard the results of my question about 2/4
The network driver now looks for the vlan element in network and portgroup objects, and logs an error at network define time if a vlan is requested for a network type that doesn't support it. (Currently vlan configuration is only supported for openvswitch networks, and networks used to do hostdev assignment of SR-IOV VFs.) At runtime, the three potential sources of vlan information are examined in this order: interface, chosen portgroup, network, and the first that is non-empty is used. Another check for valid network type is made at this time, since the interface may have requested a vlan (a legal thing to have in the interface config, since it's not known until runtime if the chosen network will actually support it). Since we must also check for domains requesting vlans for unsupported connection types even if they are type='network', and since networkAllocateActualDevice() is being called in exactly the correct places, and has all of the necessary information to check, I slightly modified the logic of that function so that interfaces that aren't type='network' don't just return immediately. Instead, they also perform all the same validation for supported features. Because of this, it's not necessary to make this identical check in the other three places that would normally require it: 1) qemu domain startup, 2) qemu device hotplug, 3) lxc domain startup. This can be seen as a first step in consolidating network-related functionality into the network driver, rather than having copies of the same code spread around in multiple places; this will make it easier to split the network parts off into a separate deamon, as we've discussed recently. --- src/network/bridge_driver.c | 114 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 104 insertions(+), 10 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index b784eb4..2d453d0 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2246,6 +2246,42 @@ cleanup: } +static int +networkValidate(virNetworkDefPtr def) +{ + int ii; + bool vlanUsed, vlanAllowed; + + /* The only type of networks that currently support transparent + * vlan configuration are those using hostdev sr-iov devices from + * a pool, and those using an openvswitch bridge. + */ + + vlanAllowed = (def->forwardType == VIR_NETWORK_FORWARD_BRIDGE && + def->virtPortProfile && + def->virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH); + + vlanUsed = def->vlan.nTags > 0; + for (ii = 0; ii < def->nPortGroups && !(vlanUsed && vlanAllowed); ii++) { + if (def->portGroups[ii].vlan.nTags > 0) + vlanUsed = true; + if (def->forwardType == VIR_NETWORK_FORWARD_BRIDGE && + def->portGroups[ii].virtPortProfile && + (def->portGroups[ii].virtPortProfile->virtPortType + == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)) { + vlanAllowed = true; + } + } + if (vlanUsed && !vlanAllowed) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("<vlan> element specified for network %s, " + "whose type doesn't support vlan configuration"), + def->name); + return -1; + } + return 0; +} + static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { struct network_driver *driver = conn->networkPrivateData; virNetworkDefPtr def; @@ -2273,6 +2309,9 @@ static virNetworkPtr networkCreate(virConnectPtr conn, const char *xml) { virNetworkSetBridgeMacAddr(def); } + if (networkValidate(def) < 0) + goto cleanup; + if (!(network = virNetworkAssignDef(&driver->networks, def))) goto cleanup; @@ -2346,6 +2385,9 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) { } } + if (networkValidate(def) < 0) + goto cleanup; + if (!(network = virNetworkAssignDef(&driver->networks, def))) goto cleanup; @@ -2744,18 +2786,24 @@ int networkAllocateActualDevice(virDomainNetDefPtr iface) { struct network_driver *driver = driverState; - virNetworkObjPtr network; - virNetworkDefPtr netdef; - virPortGroupDefPtr portgroup; - virNetDevVPortProfilePtr virtport = NULL; + enum virDomainNetType actualType = iface->type; + virNetworkObjPtr network = NULL; + virNetworkDefPtr netdef = NULL; + virPortGroupDefPtr portgroup = NULL; + virNetDevVPortProfilePtr virtport = iface->virtPortProfile; + virNetDevVlanPtr vlan = NULL; virNetworkForwardIfDefPtr dev = NULL; unsigned int num_virt_fns = 0; char **vfname = NULL; int ii; int ret = -1; + /* it's handy to have this initialized if we skip directly to validate */ + if (iface->vlan.nTags > 0) + vlan = &iface->vlan; + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) - return 0; + goto validate; virDomainActualNetDefFree(iface->data.network.actual); iface->data.network.actual = NULL; @@ -2815,7 +2863,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) goto error; } - iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_BRIDGE; + iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_BRIDGE; iface->data.network.actual->data.bridge.brname = strdup(netdef->bridge); if (!iface->data.network.actual->data.bridge.brname) { virReportOOMError(); @@ -2861,7 +2909,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) } /* Set type=direct and appropriate <source mode='xxx'/> */ - iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_DIRECT; + iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_DIRECT; switch (netdef->forwardType) { case VIR_NETWORK_FORWARD_BRIDGE: iface->data.network.actual->data.direct.mode = VIR_NETDEV_MACVLAN_MODE_BRIDGE; @@ -2999,6 +3047,49 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) if (virNetDevVPortProfileCheckComplete(virtport, true) < 0) goto error; + /* copy appropriate vlan info to actualNet */ + if (iface->vlan.nTags > 0) + vlan = &iface->vlan; + else if (portgroup && portgroup->vlan.nTags > 0) + vlan = &portgroup->vlan; + else if (netdef && netdef->vlan.nTags > 0) + vlan = &netdef->vlan; + + if (virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0) + goto error; + +validate: + /* make sure that everything now specified for the device is + * actually supported on this type of network. NB: network, + * netdev, and iface->data.network.actual may all be NULL. + */ + + if (vlan) { + /* vlan configuration via libvirt is only supported for + * PCI Passthrough SR-IOV devices and openvswitch bridges. + * otherwise log an error and fail + */ + if (!(actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV || + (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE && + virtport && virtport->virtPortType + == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH))) { + if (netdef) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("an interface connecting to network '%s' " + "is requesting a vlan tag, but that is not " + "supported for this type of network"), + netdef->name); + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("an interface of type '%s' " + "is requesting a vlan tag, but that is not " + "supported for this type of connection"), + virDomainNetTypeToString(iface->type)); + } + goto error; + } + } + if (dev) { /* we are now assured of success, so mark the allocation */ dev->connections++; @@ -3006,10 +3097,13 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) dev->dev, dev->connections); } - netdef->connections++; - VIR_DEBUG("Using network %s, %d connections", - netdef->name, netdef->connections); + if (netdef) { + netdef->connections++; + VIR_DEBUG("Using network %s, %d connections", + netdef->name, netdef->connections); + } ret = 0; + error: for (ii = 0; ii < num_virt_fns; ii++) VIR_FREE(vfname[ii]); -- 1.7.11.2
On 08/14/2012 01:15 AM, Laine Stump wrote:
The network driver now looks for the vlan element in network and portgroup objects, and logs an error at network define time if a vlan is requested for a network type that doesn't support it. (Currently vlan configuration is only supported for openvswitch networks, and networks used to do hostdev assignment of SR-IOV VFs.)
At runtime, the three potential sources of vlan information are examined in this order: interface, chosen portgroup, network, and the first that is non-empty is used. Another check for valid network type is made at this time, since the interface may have requested a vlan (a legal thing to have in the interface config, since it's not known until runtime if the chosen network will actually support it).
Since we must also check for domains requesting vlans for unsupported connection types even if they are type='network', and since networkAllocateActualDevice() is being called in exactly the correct places, and has all of the necessary information to check, I slightly modified the logic of that function so that interfaces that aren't type='network' don't just return immediately. Instead, they also perform all the same validation for supported features. Because of this, it's not necessary to make this identical check in the other three places that would normally require it: 1) qemu domain startup, 2) qemu device hotplug, 3) lxc domain startup.
This can be seen as a first step in consolidating network-related functionality into the network driver, rather than having copies of the same code spread around in multiple places; this will make it easier to split the network parts off into a separate deamon, as we've
s/deamon/daemon/
discussed recently. --- src/network/bridge_driver.c | 114 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 104 insertions(+), 10 deletions(-)
Not much cleanup yet, but I agree with the idea of consolidating things into common routines where possible, and this does lay a groundwork for that.
+static int +networkValidate(virNetworkDefPtr def) +{ + int ii; + bool vlanUsed, vlanAllowed; + + /* The only type of networks that currently support transparent + * vlan configuration are those using hostdev sr-iov devices from + * a pool, and those using an openvswitch bridge.
Spelling of openvswitch. ACK with nits fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Aug 14, 2012 at 2:15 AM, Laine Stump <laine@laine.org> wrote:
danpb raised a (valid) objection about the XML format in the first version of these patches, so I've redone it to be like this:
<vlan trunk='yes'> <tag id='42'/> <tag id='30'/> </vlan>
The downside is that for a single vlan tag, it's more verbose:
<vlan> <tag id='42'/> </vlan>
On the other hand, it's more consistent.
The rest of the patches in the series were unchanged (and even the data definition is unchanged).
Laine, Thank you for your work on the vlan interface code.
On 08/14/2012 03:15 AM, Laine Stump wrote:
The purpose of these patches is to define a data type that can properly describe an interface's vlan configuration (including multiple vlans for trunking), provide XML parser and formatter to translate to and from the new data type, add the new <vlan> element to both domain <interface> and to <network> in appropriate places, and to provide the new vlan data to the hypervisor driver.
Thanks to everyone for their reviews and comments. I've pushed these 4 patches, so now Kyle can create a (hopefully much simplified) patch that looks for virDomainNetGetActualVlan(netdef) and setups up the vlan appropriately for guests connecting to an Open vSwtich network. I also have a (very small, because almost everything was already there) patch to support setting the vlan tag on SR-IOV PCI passthrough devices. I still have to get a successful build on my SR-IOV-capable hardware so I can test it, though :-)
On Aug 15, 2012, at 12:47 PM, Laine Stump wrote:
On 08/14/2012 03:15 AM, Laine Stump wrote:
The purpose of these patches is to define a data type that can properly describe an interface's vlan configuration (including multiple vlans for trunking), provide XML parser and formatter to translate to and from the new data type, add the new <vlan> element to both domain <interface> and to <network> in appropriate places, and to provide the new vlan data to the hypervisor driver.
Thanks to everyone for their reviews and comments. I've pushed these 4 patches, so now Kyle can create a (hopefully much simplified) patch that looks for virDomainNetGetActualVlan(netdef) and setups up the vlan appropriately for guests connecting to an Open vSwtich network.
I also have a (very small, because almost everything was already there) patch to support setting the vlan tag on SR-IOV PCI passthrough devices. I still have to get a successful build on my SR-IOV-capable hardware so I can test it, though :-)
Thanks for your work on these patches Laine! I'll go ahead and reformat my patch to add VLAN support to Open vSwitch virtual port types and resubmit. Thanks, Kyle
participants (4)
-
Dennis Jenkins -
Eric Blake -
Kyle Mestery (kmestery) -
Laine Stump