[libvirt] [RFC Incomplete Patch] Libvirt + Openvswitch
Laine Stump
laine at laine.org
Fri Jan 27 20:42:55 UTC 2012
On 01/27/2012 05:58 AM, Dan Wendlandt wrote:
> Hello all,
>
> I know of many people who want to spin up VMs using libvirt + kvm/qemu
> and attach those VMs to an openvswitch bridge (see:
> http://www.openvswitch.org). However, the only way I know of to get
> this working is a kludge that uses to tap devices along with
> <interface type="ethernet"> while running ovs-vsctl outside of
> libvirt. Even worse, doing this on RHEL/Fedora seems to require
> privilege tweaks (e.g., running qemu as root, not dropping
> capabilities), which may not be acceptable for production deployments
> (see:
> http://fedoraproject.org/wiki/How_to_debug_Virtualization_problems#Errors_using_.3Cinterface_type.3D.27ethernet.27.2F.3E
> ).
>
> So I would like to start taking steps toward better
> libvirt/openvswitch integration. My initial step has the fairly limit
> goal of enabling kvm/qemu VM NICs to attach to an openvswitch bridge
> in much the same way VM NIC can already attached to the linux bridge.
> For example, specifying:
>
> <interface type="openvswitch">
> <source bridge="br0"/>
> <mac address="ca:fe:de;ad:be:ef"/>
> </interface>
>
> I also wanted to add a new child element of <interface> that can be
> used to specify some OVS specific configuration. To start, I just
> want to expose a single 'interfaceid' value (this parameter is
> specific to openvswitch). Extending the previous example:
>
> <interface type="openvswitch">
> <source bridge="br0"/>
> <mac address="ca:fe:de;ad:be:ef"/>
> <openvswitchport interfaceid="interface-xyz"/>
> </interface>
This is a good start!
It might be better to use the same virtualport element that is used for
the various macvtap flavors, with a different type attribute, which
would be used to determine which of attributes in the <parameters>
sub-element to use - more or less how kmestery suggested in his followup
proposing XML for network configuration. A consistency like this tends
to result in less new code / data structures.
(btw, you say that "interfaceid" is specific to openvswitch, but that's
a generic enough name that it might be useful for someone else. Or maybe
you could even re-use something from <virtualport>, maybe instanceid,
which is also unique per interface)
>
> For this first step (see attached patch), I am only targeting the
> model where the OVS bridge has been created externally ahead of time.
> I am not tackling any of the "network" logic that actually
> creates/destroys bridges, as it is not needed for my target use case.
>
> A couple notes about the attached patch:
> - I haven't actually run this code. I was just poking around the
> libvirt code to learn about it and figured that a diff was the most
> concrete way to propose what I was thinking of doing. I would be
> curious for pointers about big chunks of work that I may have missed
> (for example, I haven't added any tests yet).
> - the code was modeled on the network interface "bandwidth" code, that
> calls out to 'tc' to configure bandwidth rates. Ideally we would be
> able to make direct C calls to OVS (and we plan to make that possible
> in the future), but calling an external utility right now is the only
> viable path.
> - no attention was paid to style guidelines. Will do that before any
> real submission.
> - I wasn't very clear on the notion of an "actual" net def, as opposed
> to a normal net def. What's the best place to look for documentation
> on this?
The ActualNetDef was invented as a way to retain the original config of
the interface at runtime, while also providing the details of what kind
of interface setup that config resulted in. So the NetDef might say the
interface type is "network", but the ActualNetDef would reflect that the
interface ended up using eth25 in direct (macvtap) private mode, for
example. The <actual> element is only used internally to keep track of
what resources are in use by a domain even in the case of libvirtd
restarting (this is stored in xml files in libvirt's "statusdir"), and
are never seen in the dumpxml returned by the libvirt public API, so you
won't find documentation for them in the public API docs.
In your case, until there is an openvswitch *network* type, the
ActualNetDef won't be used for openvswitch interfaces.
When there is an openvswitch network type, if an interface is of
type='network', then the qemu domain startup code will call
networkAllocateActualDevice(), which will look up the network, determine
its type, allocate the necessary resources, and fill in the ActualNetDef
to return to qemu. So, any resources that are specific to a particular
type of interface, or could be filled in from a portgroup entry (e.g.
bandwidth, virtualport) should also exist in the ActualNetDef, and a
helper function made that returns the value from the ActualNetDef if
it's there, or from the NetDef otherwise.
For example, a network that is forward mode='passthrough' (indicating
macvtap passthrough mode) may have a pool of devices for use by domains.
Each domain's config only says type='network' network name='mynet', but
at connect time, networkAllocateActualDevice is called, which picks an
unused device and fills that into a new ActualNetDef (along with any
common virtualport/bandwidth info from an appropriate portgroup), and
returns that to the qemu driver. Now the qemu driver has the info that
it will use interface "ethxx" in macvtap passthrough mode (by looking at
the ActualNetDef), but the original config remains unspoiled.
>
> Anyway, I'm primarily looking for feedback about whether I'm barking
> up the right tree before I spend time debugging or polishing the
> patch, so I would appreciate thoughts, advice, etc. Thanks,
>
> Dan
>
>
> --
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Dan Wendlandt
> Nicira Networks: www.nicira.com <http://www.nicira.com>
> twitter: danwendlandt
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> ovs.diff
>
>
> diff --git a/configure.ac b/configure.ac
> index 6709ebf..709128d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -213,6 +213,8 @@ AC_PATH_PROG([UDEVSETTLE], [udevsettle], [],
> [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
> AC_PATH_PROG([MODPROBE], [modprobe], [],
> [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
> +AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl],
> + [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
>
> AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"],
> [Location or name of the dnsmasq program])
> @@ -220,6 +222,9 @@ AC_DEFINE_UNQUOTED([RADVD],["$RADVD"],
> [Location or name of the radvd program])
> AC_DEFINE_UNQUOTED([TC],["$TC"],
> [Location or name of the tc profram (see iproute2)])
> +AC_DEFINE_UNQUOTED([OVSVSCTL],["$OVSVSCTL"],
> + [Location or name of the ovs-vsctl program])
> +
> if test -n "$UDEVADM"; then
> AC_DEFINE_UNQUOTED([UDEVADM],["$UDEVADM"],
> [Location or name of the udevadm program])
> diff --git a/src/Makefile.am b/src/Makefile.am
> index a44446f..679a060 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -94,6 +94,7 @@ UTIL_SOURCES = \
> util/virkeymaps.h \
> util/virnetdev.h util/virnetdev.c \
> util/virnetdevbandwidth.h util/virnetdevbandwidth.c \
> + util/virnetdevopenvswitch.h util/virnetdevopenvswitch.c \
> util/virnetdevbridge.h util/virnetdevbridge.c \
> util/virnetdevmacvlan.c util/virnetdevmacvlan.h \
> util/virnetdevtap.h util/virnetdevtap.c \
> @@ -133,6 +134,7 @@ LOCK_DRIVER_SANLOCK_SOURCES = \
>
> NETDEV_CONF_SOURCES = \
> conf/netdev_bandwidth_conf.h conf/netdev_bandwidth_conf.c \
> + conf/netdev_openvswitch_conf.h conf/netdev_openvswitch_conf.c \
> conf/netdev_vport_profile_conf.h conf/netdev_vport_profile_conf.c
>
> # XML configuration format handling sources
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e872396..eafcd9a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -50,6 +50,7 @@
> #include "count-one-bits.h"
> #include "secret_conf.h"
> #include "netdev_vport_profile_conf.h"
> +#include "netdev_openvswitch_conf.h"
> #include "netdev_bandwidth_conf.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN
> @@ -279,6 +280,7 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST,
> "network",
> "bridge",
> "internal",
> + "openvswitch",
> "direct")
>
> VIR_ENUM_IMPL(virDomainNetBackend, VIR_DOMAIN_NET_BACKEND_TYPE_LAST,
> @@ -945,6 +947,10 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> VIR_FREE(def->data.bridge.brname);
> break;
> + case VIR_DOMAIN_NET_TYPE_OPENVSWITCH:
> + VIR_FREE(def->data.ovs.ovsPort->brname);
> + VIR_FREE(def->data.ovs.ovsPort->InterfaceID);
> + VIR_FREE(def->data.ovs.ovsPort);
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> VIR_FREE(def->data.direct.linkdev);
> VIR_FREE(def->data.direct.virtPortProfile);
> @@ -998,6 +1004,12 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
> VIR_FREE(def->data.direct.virtPortProfile);
> break;
>
> + case VIR_DOMAIN_NET_TYPE_OPENVSWITCH:
> + VIR_FREE(def->data.ovs.ovsPort->brname);
> + VIR_FREE(def->data.ovs.ovsPort->InterfaceID);
> + VIR_FREE(def->data.ovs.ovsPort);
> + break;
> +
> case VIR_DOMAIN_NET_TYPE_USER:
> case VIR_DOMAIN_NET_TYPE_LAST:
> break;
> @@ -3606,6 +3618,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> goto error;
> }
> if (actual->type != VIR_DOMAIN_NET_TYPE_BRIDGE&&
> + actual->type != VIR_DOMAIN_NET_TYPE_OPENVSWITCH&&
> actual->type != VIR_DOMAIN_NET_TYPE_DIRECT&&
> actual->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -3616,6 +3629,14 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
>
> if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> actual->data.bridge.brname = virXPathString("string(./source[1]/@bridge)", ctxt);
> + } else if (actual->type == VIR_DOMAIN_NET_TYPE_OPENVSWITCH) {
> + actual->data.bridge.brname = virXPathString("string(./source[1]/@bridge)", ctxt);
> +
> + xmlNodePtr ovsPortNode = virXPathNode("./openvswitchport", ctxt);
> + if (ovsPortNode&&
> + (!(actual->data.ovs.ovsPort =
> + virNetDevOpenvswitchPortParse(ovsPortNode))))
> + goto error;
> } else if (actual->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> xmlNodePtr virtPortNode;
>
> @@ -3695,6 +3716,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
> char *linkstate = NULL;
> virNWFilterHashTablePtr filterparams = NULL;
> virNetDevVPortProfilePtr virtPort = NULL;
> + virNetDevOpenvswitchPortPtr ovsPort = NULL;
> virDomainActualNetDefPtr actual = NULL;
> xmlNodePtr oldnode = ctxt->node;
> int ret;
> @@ -3736,6 +3758,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
> (def->type == VIR_DOMAIN_NET_TYPE_BRIDGE)&&
> (xmlStrEqual(cur->name, BAD_CAST "source"))) {
> bridge = virXMLPropString(cur, "bridge");
> + } else if ((network == NULL)&&
> + (def->type == VIR_DOMAIN_NET_TYPE_OPENVSWITCH)&&
> + (xmlStrEqual(cur->name, BAD_CAST "source"))) {
> + bridge = virXMLPropString(cur, "bridge");
you could just add an extra clause to the previous conditional. But
that's a detail for later...
> } else if ((dev == NULL)&&
> (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> def->type == VIR_DOMAIN_NET_TYPE_DIRECT)&&
> @@ -3748,6 +3774,11 @@ virDomainNetDefParseXML(virCapsPtr caps,
> xmlStrEqual(cur->name, BAD_CAST "virtualport")) {
> if (!(virtPort = virNetDevVPortProfileParse(cur)))
> goto error;
> + } else if ((ovsPort == NULL)&&
> + ((def->type == VIR_DOMAIN_NET_TYPE_OPENVSWITCH)&&
> + xmlStrEqual(cur->name, BAD_CAST "openvswitchport"))) {
> + if (!(ovsPort = virNetDevOpenvswitchPortParse(cur)))
> + goto error;
Again, I think I prefer the idea of re-using <virtualport> instead of
creating a new element type.
> } else if ((network == NULL)&&
> ((def->type == VIR_DOMAIN_NET_TYPE_SERVER) ||
> (def->type == VIR_DOMAIN_NET_TYPE_CLIENT) ||
> @@ -3885,6 +3916,14 @@ virDomainNetDefParseXML(virCapsPtr caps,
> }
> break;
>
> + case VIR_DOMAIN_NET_TYPE_OPENVSWITCH:
> + if (bridge == NULL) {
> + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("No<source> 'bridge' attribute specified with<interface type='openvswitch'/>"));
> + goto error;
> + }
> + break;
> +
> case VIR_DOMAIN_NET_TYPE_CLIENT:
> case VIR_DOMAIN_NET_TYPE_SERVER:
> case VIR_DOMAIN_NET_TYPE_MCAST:
> @@ -10279,6 +10318,7 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> }
>
> if (def->type != VIR_DOMAIN_NET_TYPE_BRIDGE&&
> + def->type != VIR_DOMAIN_NET_TYPE_OPENVSWITCH&&
> def->type != VIR_DOMAIN_NET_TYPE_DIRECT&&
> def->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -10312,6 +10352,14 @@ virDomainActualNetDefFormat(virBufferPtr buf,
> goto error;
> virBufferAdjustIndent(buf, -8);
> break;
> + case VIR_DOMAIN_NET_TYPE_OPENVSWITCH:
> + virBufferEscapeString(buf, "<source bridge='%s'/>\n",
> + def->data.ovs.ovsPort->brname);
> + virBufferAdjustIndent(buf, 6);
> + if (virNetDevOpenvswitchPortFormat(def->data.ovs.ovsPort, buf)< 0)
> + return -1;
> + virBufferAdjustIndent(buf, -6);
> + break;
> default:
> break;
> }
> @@ -10408,6 +10456,14 @@ virDomainNetDefFormat(virBufferPtr buf,
> virBufferAdjustIndent(buf, -6);
> break;
>
> + case VIR_DOMAIN_NET_TYPE_OPENVSWITCH:
> + virBufferEscapeString(buf, "<source bridge='%s'/>\n",
> + def->data.ovs.ovsPort->brname);
> + virBufferAdjustIndent(buf, 6);
> + if (virNetDevOpenvswitchPortFormat(def->data.ovs.ovsPort, buf)< 0)
> + return -1;
> + virBufferAdjustIndent(buf, -6);
> + break;
> case VIR_DOMAIN_NET_TYPE_USER:
> case VIR_DOMAIN_NET_TYPE_LAST:
> break;
> @@ -13729,6 +13785,18 @@ virDomainNetGetActualBandwidth(virDomainNetDefPtr iface)
> return iface->bandwidth;
> }
>
> +virNetDevOpenvswitchPortPtr
> +virDomainNetGetActualOpenvswitchPortPtr(virDomainNetDefPtr iface)
> +{
> + if (iface->type == VIR_DOMAIN_NET_TYPE_OPENVSWITCH)
> + return iface->data.ovs.ovsPort;
> + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> + return NULL;
> + if (!iface->data.network.actual)
> + return NULL;
> + return iface->data.network.actual->data.ovs.ovsPort;
> +}
> +
Right. Even though you claim you didn't understand it, and you wouldn't
*need* it right away, you've done a reasonable job of filling out the
actualnetdef for this new type :-)
>
> /* 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 3b522a9..5d0cfcd 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -41,6 +41,7 @@
> # include "virnetdevmacvlan.h"
> # include "sysinfo.h"
> # include "virnetdevvportprofile.h"
> +# include "virnetdevopenvswitch.h"
> # include "virnetdevbandwidth.h"
>
> /* Different types of hypervisor */
> @@ -525,6 +526,7 @@ enum virDomainNetType {
> VIR_DOMAIN_NET_TYPE_BRIDGE,
> VIR_DOMAIN_NET_TYPE_INTERNAL,
> VIR_DOMAIN_NET_TYPE_DIRECT,
> + VIR_DOMAIN_NET_TYPE_OPENVSWITCH,
>
> VIR_DOMAIN_NET_TYPE_LAST,
> };
> @@ -574,6 +576,9 @@ struct _virDomainActualNetDef {
> int mode; /* enum virMacvtapMode from util/macvtap.h */
> virNetDevVPortProfilePtr virtPortProfile;
> } direct;
> + struct {
> + virNetDevOpenvswitchPortPtr ovsPort;
If you know that the ovsPort will always be valid any time
type='openvswitch', it would make memory management/error recovery
simpler to just make this virNetDevOpenvswithcPort, rather than a
pointer to one. Normally a pointer to a struct is used if something is
optional (so NULL will indicate that it was missing from the config), or
if someone just has fun doing malloc/free calisthenics ;-)
> + } ovs;
> } data;
> virNetDevBandwidthPtr bandwidth;
> };
> @@ -628,6 +633,9 @@ struct _virDomainNetDef {
> int mode; /* enum virMacvtapMode from util/macvtap.h */
> virNetDevVPortProfilePtr virtPortProfile;
> } direct;
> + struct {
> + virNetDevOpenvswitchPortPtr ovsPort;
Same here.
> + } ovs;
> } data;
> struct {
> bool sndbuf_specified;
> @@ -1860,6 +1868,9 @@ virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface);
> virNetDevBandwidthPtr
> virDomainNetGetActualBandwidth(virDomainNetDefPtr iface);
>
> +virNetDevOpenvswitchPortPtr
> +virDomainNetGetActualOpenvswitchPortPtr(virDomainNetDefPtr iface);
> +
> int virDomainControllerInsert(virDomainDefPtr def,
> virDomainControllerDefPtr controller);
> void virDomainControllerInsertPreAlloced(virDomainDefPtr def,
> diff --git a/src/conf/netdev_openvswitch_conf.c b/src/conf/netdev_openvswitch_conf.c
> new file mode 100644
> index 0000000..f83b8bf
> --- /dev/null
> +++ b/src/conf/netdev_openvswitch_conf.c
> @@ -0,0 +1,75 @@
> +/*
> + * Copyright (C) 2012 Nicira, 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Authors:
> + * Dan Wendlandt<dan at nicira.com>
> + */
> +
> +#include<config.h>
> +
> +#include "netdev_openvswitch_conf.h"
> +#include "virterror_internal.h"
> +#include "memory.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +#define virNetDevError(code, ...) \
> + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
> + __FUNCTION__, __LINE__, __VA_ARGS__)
> +
> +
> +virNetDevOpenvswitchPortPtr
> +virNetDevOpenvswitchPortParse(xmlNodePtr node)
> +{
> + virNetDevOpenvswitchPortPtr ovsPort = NULL;
> + xmlNodePtr cur = node->children;
> +
> + if (VIR_ALLOC(ovsPort)< 0) {
> + virReportOOMError();
> + return NULL;
> + }
> +
> + ovsPort->InterfaceID = virXMLPropString(node, "interfaceid");
> +
> + if (!ovsPort->InterfaceID || strlen(ovsPort->InterfaceID) == 0) {
> + // interfaceID does not have to be a UUID,
> + // but a UUID is a reasonable default
> + virAlloc(ovsPort->InterfaceID, VIR_UUID_STRING_BUFLEN);
> + if (virUUIDGenerate(ovsPort->InterfaceID)) {
> + virNetDevError(VIR_ERR_XML_ERROR, "%s",
> + _("cannot generate a random uuid for interfaceid"));
> + goto error;
> + }
> + }
> +
> +cleanup:
> + return ovsPort;
> +
> +error:
> + VIR_FREE(ovsPort);
> + goto cleanup;
> +}
> +
> +
> +int
> +virNetDevOpenvswitchPortFormat(virNetDevOpenvswitchPortPtr ovsPort,
> + virBufferPtr buf)
> +{
> + if (ovsPort)
> + virBufferAsprintf(buf, "<openvswitchport interfaceid='%s'/>\n",
> + ovsPort->InterfaceID);
> + return 0;
> +}
> diff --git a/src/conf/netdev_openvswitch_conf.h b/src/conf/netdev_openvswitch_conf.h
> new file mode 100644
> index 0000000..d0d077b
> --- /dev/null
> +++ b/src/conf/netdev_openvswitch_conf.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) 2012 Nicira, 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Authors:
> + * Dan Wendlandt<dan at nicira.com>
> + */
> +
> +#ifndef __VIR_NETDEV_OPENVSWITCH_CONF_H__
> +# define __VIR_NETDEV_OPENVSWITCH_CONF_H__
> +
> +# include "internal.h"
> +# include "virnetdevopenvswitch.h"
> +# include "buf.h"
> +# include "xml.h"
> +
> +virNetDevOpenvswitchPortPtr
> +virNetDevOpenvswitchPortParse(xmlNodePtr node);
> +
> +int
> +virNetDevOpenvswitchPortFormat(virNetDevOpenvswitchPortPtr ovsPort,
> + virBufferPtr buf);
> +
> +
> +#endif /* __VIR_NETDEV_OPENVSWITCH_CONF_H__ */
For something this short, it may not be worth having an extra file
(requiring the extra export symbols, etc) - it could just be added to
domain_conf.h (of course someone else may be planning to split up
domain_conf.h as we speak, so...)
Of course, if you follow the advice of re-using virtualport for the
interfaceID, then you won't need this anyway.
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 04ae35c..1aa78a7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -378,6 +378,7 @@ virDomainNetGetActualBridgeName;
> virDomainNetGetActualDirectDev;
> virDomainNetGetActualDirectMode;
> virDomainNetGetActualDirectVirtPortProfile;
> +virDomainNetGetActualOpenvswitchPortPtr;
> virDomainNetGetActualType;
> virDomainNetIndexByMac;
> virDomainNetInsert;
> @@ -741,6 +742,10 @@ nlComm;
> virNetDevBandwidthFormat;
> virNetDevBandwidthParse;
>
> +# netdev_openvswitch_conf.h
> +virNetDevOpenvswitchPortFormat;
> +virNetDevOpenvswitchPortParse;
> +
>
> # netdev_vportprofile_conf.h
> virNetDevVPortProfileFormat;
> @@ -1238,6 +1243,10 @@ virNetDevMacVLanCreateWithVPortProfile;
> virNetDevMacVLanDeleteWithVPortProfile;
>
>
> +# virnetdevopenvswitch.h
> +virNetDevOpenvswitchAddPort;
> +
> +
> # virnetdevtap.h
> virNetDevTapCreate;
> virNetDevTapCreateInBridgePort;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 5d0d528..d4989c8 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1766,7 +1766,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
> goto err0;
> }
> if (virNetDevTapCreateInBridgePort(network->def->bridge,
> -&macTapIfName, network->def->mac, 0, false, NULL)< 0) {
> +&macTapIfName, network->def->mac, 0, false, NULL, NULL)< 0) {
> VIR_FREE(macTapIfName);
> goto err0;
> }
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index aaccf62..6f411c8 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -221,6 +221,13 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> virReportOOMError();
> return -1;
> }
> + } else if (actualType == VIR_DOMAIN_NET_TYPE_OPENVSWITCH) {
> + virNetDevOpenvswitchPortPtr p =
> + virDomainNetGetActualOpenvswitchPortPtr(net);
> + if (!(brname = strdup(p->brname))) {
> + virReportOOMError();
> + return -1;
> + }
> } else {
> qemuReportError(VIR_ERR_INTERNAL_ERROR,
> _("Network type %d is not supported"),
> @@ -247,7 +254,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
> tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */
> err = virNetDevTapCreateInBridgePort(brname,&net->ifname, tapmac,
> - vnet_hdr, true,&tapfd);
> + vnet_hdr, true,&tapfd, net);
> virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0);
> if (err< 0) {
> if (template_ifname)
> @@ -2539,6 +2546,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
> switch (netType) {
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> + case VIR_DOMAIN_NET_TYPE_OPENVSWITCH:
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> virBufferAddLit(&buf, "tap");
> virBufferAsprintf(&buf, "%cfd=%s", type_sep, tapfd);
> @@ -2587,6 +2595,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
> case VIR_DOMAIN_NET_TYPE_ETHERNET:
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> + case VIR_DOMAIN_NET_TYPE_OPENVSWITCH:
> case VIR_DOMAIN_NET_TYPE_INTERNAL:
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> case VIR_DOMAIN_NET_TYPE_LAST:
> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> index 316d558..4ad2d90 100644
> --- a/src/uml/uml_conf.c
> +++ b/src/uml/uml_conf.c
> @@ -142,7 +142,7 @@ umlConnectTapDevice(virConnectPtr conn,
> memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
> tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */
> if (virNetDevTapCreateInBridgePort(bridge,&net->ifname, tapmac,
> - 0, true, NULL)< 0) {
> + 0, true, NULL, net)< 0) {
> if (template_ifname)
> VIR_FREE(net->ifname);
> goto error;
> @@ -238,6 +238,7 @@ umlBuildCommandLineNet(virConnectPtr conn,
> }
>
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> + case VIR_DOMAIN_NET_TYPE_OPENVSWITCH:
> if (umlConnectTapDevice(conn, vm, def,
> def->data.bridge.brname)< 0)
> goto error;
> diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> new file mode 100644
> index 0000000..0c96116
> --- /dev/null
> +++ b/src/util/virnetdevopenvswitch.c
> @@ -0,0 +1,79 @@
> +/*
> + * Copyright (C) 2012 Nicira, 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Authors:
> + * Dan Wendlandt<dan at nicira..com>
> + */
> +
> +#include<config.h>
> +
> +#include "virnetdevopenvswitch.h"
> +#include "command.h"
> +#include "memory.h"
> +#include "virterror_internal.h"
> +#include "ignore-value.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +/**
> + * virNetDevOpenvswitchAddPort:
> + * @brname: the bridge name
> + * @ifname: the network interface name
> + * @macaddr: the mac address of the virtual interface
> + *
> + * Adds an interface to an OVS bridge
> + *
> + * Returns 0 in case of success or an errno code in case of failure.
> + */
Actually you're returning -1 in case of failure. If you were going to
return errno, you should instead return -errno, which is a standard that
we're trying to adopt in all the libvirt code.
> +int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
> + const unsigned char *macaddr,
> + virNetDevOpenvswitchPortPtr ovs_port)
> +{
> + int ret = -1;
> + virCommandPtr cmd = NULL;
> + char *attachedmac_ex_id = NULL;
> + char *ifaceid_ex_id = NULL;
> +
> + if (virAsprintf(&attachedmac_ex_id, "external-ids:attached-mac=%s", macaddr)< 0)
> + goto cleanup;
> + if (virAsprintf(&ifaceid_ex_id, "external-ids:iface-id=%s", ovs_port->InterfaceID)< 0)
> + goto cleanup;
> +
> + cmd = virCommandNew(OVSVSCTL);
> + virCommandAddArgList(cmd, "--", "--may-exist", "add-port",
> + brname, ifname,
> + "--", "set", "Interface", ifname, attachedmac_ex_id,
> + "--", "set", "Interface", ifname, ifaceid_ex_id,
> + "--", "set", "Interface", ifname,
> + "external-ids:iface-status=active",
> + NULL);
> +
> + if (virCommandRun(cmd, NULL)< 0)
> + {
> + virReportSystemError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to add port %s to OVS bridge %s"), ifname, brname);
You'll need to use a different log function - virReportSystemError
expects its first arg to be a valid errno, but virCommandRun returns -1
on failure, and errno has already been reset by now.
> + goto cleanup;
> + }
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(attachedmac_ex_id);
> + VIR_FREE(ifaceid_ex_id);
> + virCommandFree(cmd);
> + return ret;
> +}
> +
> diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
> new file mode 100644
> index 0000000..bc6ecb3
> --- /dev/null
> +++ b/src/util/virnetdevopenvswitch.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (C) 2012 Nicira, 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, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Authors:
> + * Dan Wendlandt<dan at nicira..com>
> + */
> +
> +#ifndef __VIR_NETDEV_OPENVSWITCH_H__
> +# define __VIR_NETDEV_OPENVSWITCH_H__
> +
> +# include "internal.h"
> +
> +typedef struct _virNetDevOpenvswitchPort virNetDevOpenvswitchPort;
> +typedef virNetDevOpenvswitchPort *virNetDevOpenvswitchPortPtr;
> +struct _virNetDevOpenvswitchPort {
> + char *brname;
> + char *InterfaceID;
> +};
> +
> +int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
> + const unsigned char *macaddr,
> + virNetDevOpenvswitchPortPtr ovs_port)
> +
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> +
> +
> +#endif /* __VIR_NETDEV_OPENVSWITCH_H__ */
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index 2ed53c6..3368051 100644
> --- a/src/util/virnetdevtap.c
> +++ b/src/util/virnetdevtap.c
> @@ -25,6 +25,7 @@
> #include "virnetdevtap.h"
> #include "virnetdev.h"
> #include "virnetdevbridge.h"
> +#include "virnetdevopenvswitch.h"
> #include "virterror_internal.h"
> #include "virfile.h"
> #include "virterror_internal.h"
> @@ -249,6 +250,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED)
> * @macaddr: desired MAC address (VIR_MAC_BUFLEN long)
> * @vnet_hdr: whether to try enabling IFF_VNET_HDR
> * @tapfd: file descriptor return value for the new tap device
> + * @net: the net descriptor
> *
> * This function creates a new tap device on a bridge. @ifname can be either
> * a fixed name or a name template with '%d' for dynamic name allocation.
> @@ -265,8 +267,16 @@ int virNetDevTapCreateInBridgePort(const char *brname,
> const unsigned char *macaddr,
> int vnet_hdr,
> bool up,
> - int *tapfd)
> + int *tapfd,
> + virDomainNetDefPtr net)
I don't think I like putting something as high level as a NetDef in the
arg list of this low level utility function. All the existing parameters
are fairly generic, so the file could be easily transported to a
different project and used with little or no modification.
Instead, you might want to have something like this:
int virNetDevTapCreateInBridgePort(const char *brname,
int type;
const unsigned char *macaddr,
const char *interfaceID,
int vnet_hdr,
bool up,
int *tapfd)
Or alternately, keep the existing function clean, and write a new one using the same building blocks.
> {
> + int actualType;
> +
> + if (net)
> + actualType = virDomainNetGetActualType(net);
> + else
> + actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
> +
> if (virNetDevTapCreate(ifname, vnet_hdr, tapfd)< 0)
> return -1;
>
> @@ -286,8 +296,14 @@ int virNetDevTapCreateInBridgePort(const char *brname,
> if (virNetDevSetMTUFromDevice(*ifname, brname)< 0)
> goto error;
>
> - if (virNetDevBridgeAddPort(brname, *ifname)< 0)
> - goto error;
> + if (actualType == VIR_DOMAIN_NET_TYPE_OPENVSWITCH) {
> + virNetDevOpenvswitchPortPtr p = virDomainNetGetActualOpenvswitchPortPtr(net);
> + if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr, p)< 0)
> + goto error;
> + } else {
> + if (virNetDevBridgeAddPort(brname, *ifname)< 0)
> + goto error;
> + }
>
> if (virNetDevSetOnline(*ifname, up)< 0)
> goto error;
> diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
> index fb35ac5..e0be898 100644
> --- a/src/util/virnetdevtap.h
> +++ b/src/util/virnetdevtap.h
> @@ -24,6 +24,7 @@
> # define __VIR_NETDEV_TAP_H__
>
> # include "internal.h"
> +# include "conf/domain_conf.h"
>
> int virNetDevTapCreate(char **ifname,
> int vnet_hdr,
> @@ -38,7 +39,8 @@ int virNetDevTapCreateInBridgePort(const char *brname,
> const unsigned char *macaddr,
> int vnet_hdr,
> bool up,
> - int *tapfd)
> + int *tapfd,
> + virDomainNetDefPtr net)
Same here.
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> ATTRIBUTE_RETURN_CHECK;
>
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list