On 02/03/2012 05:55 PM, Ansis Atteka wrote:
This patch allows libvirt to add interfaces to already
existing Open vSwitch bridges.
You guys are moving fast!
The following syntax in
domain XML file must be used:
<interface type='bridge'>
<mac address='52:54:00:d0:3f:f2'/>
<source bridge='ovsbr'/>
<virtualport type='openvswitch'>
<parameters interfaceid='921a80cd-e6de-5a2e-db9c-ab27f15a6e1d'/>
</virtualport>
<address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
</interface>
or if libvirt should auto-generate the interfaceid us following syntax:
<interface type='bridge'>
<mac address='52:54:00:d0:3f:f2'/>
<source bridge='ovsbr'/>
<virtualport type='openvswitch'>
</virtualport>
<address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
</interface>
To create Open vSwitch bridge us following command:
ovs-vsctl add-br ovsbr
1. This patch has been tested only on Ubuntu 11.10 with KVM
2. Auto-generated interfaceid is not persistent across libvirt reboots
Although I don't necessarily agree with the idea of auto-generated data
being added by the parser, there are many prerequisites for this (for
example the mac address in network devices and instanceID in
virtualportprofiles) - at the end of the parsing function, just look to
see if the field in question has been specified, and if not, generate a
random value and fill it in before returning from the parser. (as a
matter of fact, I point out below that you should be using the existing
virtualport parser/formatter, so you can directly lift the code to
parse/auto-generate interfaceID from the code that does that for
instanceID already).
The only problem I can foresee with doing it this way is that it then
will cause difficulties when we want to define a <network> that uses
openvswitch - following the xml syntax we've come up with for
interfaces, that would be indicated by having <forward mode='bridge'>
and <virtualport type='openvswitch'/>. The problem is that if we have
the parser function auto-generate an interfaceID whenever there is a
virtualport with type='openvswitch', the network's copy will have an
interfaceID, and that same ID would be returned to all guests that used
that network (which breaks the requirement that it be unique for every
guest).
Maybe somebody who is better at the persistent vs. transient stuff can
tell you a nice way to do this during the interface attach instead (you
can get a pointer to the persistent config using
virDomainLiveConfiHelperMethod(), but that is only called directly by
public APIs, so I don't know if it's kosher/necessary to call it for
this particular purpose)
---
configure.ac | 5 ++
src/Makefile.am | 2 +
src/conf/domain_conf.c | 46 ++++++++++++++
src/conf/domain_conf.h | 6 ++
src/conf/netdev_openvswitch_conf.c | 94 +++++++++++++++++++++++++++++
src/conf/netdev_openvswitch_conf.h | 39 ++++++++++++
src/libvirt_private.syms | 12 ++++
src/lxc/lxc_driver.c | 19 ++++--
src/network/bridge_driver.c | 3 +-
src/qemu/qemu_command.c | 3 +-
src/qemu/qemu_hotplug.c | 7 ++-
src/qemu/qemu_process.c | 3 +
src/uml/uml_conf.c | 3 +-
src/util/uuid.c | 19 ++++++
src/util/uuid.h | 1 +
src/util/virnetdevopenvswitch.c | 114 ++++++++++++++++++++++++++++++++++++
src/util/virnetdevopenvswitch.h | 46 ++++++++++++++
src/util/virnetdevtap.c | 32 +++++++++-
src/util/virnetdevtap.h | 8 ++-
19 files changed, 447 insertions(+), 15 deletions(-)
create mode 100644 src/conf/netdev_openvswitch_conf.c
create mode 100644 src/conf/netdev_openvswitch_conf.h
create mode 100644 src/util/virnetdevopenvswitch.c
create mode 100644 src/util/virnetdevopenvswitch.h
diff --git a/configure.ac b/configure.ac
index 9fb7bfc..dca178f 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])
I haven't looked at what happens if the requested binary doesn't exist
on the build system. Does it just set it to tyr "ovs-vsctl" with no
path? At least initially, people might be building this on systems which
don't have openvswitch installed, but will still want to enable support
(e.g., the official Fedora builds prior to openvswitch being available
as an official Fedora package). This will allow Fedora to ship a binary
that supports openvswitch without needing a rebuild of libvirt, in the
case that the user takes the necessary extra steps to install it. Maybe
it already does the right thing...
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 a3dd847..79a2dde 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -95,6 +95,7 @@ UTIL_SOURCES = \
util/virmacaddr.h util/virmacaddr.c \
util/virnetdev.h util/virnetdev.c \
util/virnetdevbandwidth.h util/virnetdevbandwidth.c \
+ util/virnetdevopenvswitch.h util/virnetdevopenvswitch.c \
You should try to keep lists like this in alphabetical order when it
appears that's already the case (as it is here).
util/virnetdevbridge.h util/virnetdevbridge.c \
util/virnetdevmacvlan.c util/virnetdevmacvlan.h \
util/virnetdevtap.h util/virnetdevtap.c \
@@ -136,6 +137,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 \
These two files aren't necessary - what they're doing should just be
added to netdev_vport_profile_conf.[ch]
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 aa4b32d..abec371 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -31,6 +31,7 @@
#include<sys/time.h>
#include<strings.h>
+#include "internal.h"
#include "virterror_internal.h"
#include "datatypes.h"
#include "domain_conf.h"
@@ -50,6 +51,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
@@ -952,6 +954,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
switch (def->type) {
case VIR_DOMAIN_NET_TYPE_BRIDGE:
VIR_FREE(def->data.bridge.brname);
+ VIR_FREE(def->data.bridge.ovsPort);
Since this is the first place in the code that brings up the "ovsPort",
I'll comment about it here. The idea behind re-using the <virtualport>
element was 1) to keep down the number of different elements in the XML
that the admin must learn about, but also 2) to keep down the volume of
source code needed to deal with these elements.
Rather than adding a new type of data (and a new parsing function with
associated .h and .c files), you should instead just
1) add a "struct { } openvswitch;" to the definition of
virNetDevVportProfile (in src/util/virnetdevvportprofile.h. yes, I agree
that data is defined in a too-specific location - I think it should be
in something more general (or that the other functions there that are
specific to 802.1QbX should be defined somewhere else)
2) augment the intelligence of virNetDevVPortProfileParse and
virNetDevVPortProfileFormat to populate and output that type of virtualport
3) instead of "ovsPort, include a virNetDevVportProfile in the
virDomainNetDef for the case of bridge (and of course, augment the
parser/formatter for NetDef to call virNetDevVPortProfile(Parse|Format)
at the appropriate time).
Once you do this, your new netdev_openvswitch_conf.[ch] files are
unnecessary.
While you're doing this, I think (again for consistency's sake) you
should make interfaceID
be a fixed-length uuid in binary format rather than a string (just like
instanceID), and call virUUIDFormat() at the appropriate places to turn
it into a string when you're going to include it in a commandline.
break;
case VIR_DOMAIN_NET_TYPE_DIRECT:
VIR_FREE(def->data.direct.linkdev);
@@ -995,6 +998,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
case VIR_DOMAIN_NET_TYPE_BRIDGE:
VIR_FREE(def->data.bridge.brname);
VIR_FREE(def->data.bridge.ipaddr);
+ VIR_FREE(def->data.bridge.ovsPort);
break;
case VIR_DOMAIN_NET_TYPE_INTERNAL:
@@ -3737,7 +3741,15 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
}
(It's nice that you're filling in support for the ActualNetDef now, even
though it isn't yet used - that will make it much easier to add support
for <network>s using openvswitch).
if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+ xmlNodePtr virtPortNode = virXPathNode("./virtualport", ctxt);
actual->data.bridge.brname = virXPathString("string(./source[1]/@bridge)", ctxt);
+ if (virtPortNode&& virXMLPropString(virtPortNode, "type")&&
+ STREQ(virXMLPropString(virtPortNode, "type"), "openvswitch")) {
+ if (!(actual->data.bridge.ovsPort =
+ virNetDevOpenvswitchPortParse(virtPortNode))) {
+ goto error;
+ }
+ }
Again, the code above should be replaced with a call for
virNetDevVPortProfileParse(). Just copy the code from the case of
actual->type == VIR_DOMAIN_NET_TYPE_DIRECT (but of course use
actual->data.bridge.virtPortProfile, instead of
actual->data.direct.virtPortProfile).
} else if (actual->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
xmlNodePtr virtPortNode;
@@ -3817,6 +3829,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;
@@ -3870,6 +3883,13 @@ 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_BRIDGE)&&
+ xmlStrEqual(cur->name, BAD_CAST "virtualport")&&
+ virXMLPropString(cur, "type")&&
+ STREQ(virXMLPropString(cur, "type"), "openvswitch"))) {
+ if (!(ovsPort = virNetDevOpenvswitchPortParse(cur)))
+ goto error;
Same as above.
} else if ((network == NULL)&&
((def->type == VIR_DOMAIN_NET_TYPE_SERVER) ||
(def->type == VIR_DOMAIN_NET_TYPE_CLIENT) ||
@@ -4005,6 +4025,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
def->data.bridge.ipaddr = address;
address = NULL;
}
+ def->data.bridge.ovsPort = ovsPort;
+ ovsPort = NULL;
Instead of ....bridge.ovsPort, you should have
....bridge.virPortProfile, just as is already there for
...direct.virtPortProfile.
break;
case VIR_DOMAIN_NET_TYPE_CLIENT:
@@ -10427,6 +10449,12 @@ virDomainActualNetDefFormat(virBufferPtr buf,
case VIR_DOMAIN_NET_TYPE_BRIDGE:
virBufferEscapeString(buf, "<source bridge='%s'/>\n",
def->data.bridge.brname);
+ if (def->data.bridge.ovsPort) {
+ virBufferAdjustIndent(buf, 6);
+ if (virNetDevOpenvswitchPortFormat(def->data.bridge.ovsPort, buf)< 0)
+ return -1;
+ virBufferAdjustIndent(buf, -6);
+ }
Replace with virNetDevVPortProfileFormat().
break;
case VIR_DOMAIN_NET_TYPE_DIRECT:
@@ -10514,6 +10542,12 @@ virDomainNetDefFormat(virBufferPtr buf,
if (def->data.bridge.ipaddr)
virBufferAsprintf(buf, "<ip address='%s'/>\n",
def->data.bridge.ipaddr);
+ if (def->data.bridge.ovsPort) {
+ virBufferAdjustIndent(buf, 6);
+ if (virNetDevOpenvswitchPortFormat(def->data.bridge.ovsPort, buf)< 0)
+ return -1;
+ virBufferAdjustIndent(buf, -6);
+ }
Same
break;
case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -13851,6 +13885,18 @@ virDomainNetGetActualBandwidth(virDomainNetDefPtr iface)
return iface->bandwidth;
}
+virNetDevOpenvswitchPortPtr
+virDomainNetGetActualOpenvswitchPortPtr(virDomainNetDefPtr iface)
+{
+ if (iface->type != VIR_DOMAIN_NET_TYPE_BRIDGE)
+ return NULL;
+ if (iface->data.bridge.ovsPort)
+ return iface->data.bridge.ovsPort;
+ if (!iface->data.network.actual)
+ return NULL;
+ return iface->data.network.actual->data.bridge.ovsPort;
+}
+
This function can just be removed, and the function
virDomainNetGetActualDirectVirtPortProfile:
1) renamed to the more general virDomainNetGetActualVirtPortProfile
2) modified to return the virtportprofile of the bridge if appropriate:
virNetDevVPortProfilePtr
virDomainNetGetActualVirtPortProfile(virDomainNetDefPtr iface)
{
switch (iface->type) {
case VIR_DOMAIN_NET_TYPE_DIRECT:
return iface->data.direct.virtPortProfile;
case VIR_DOMAIN_NET_TYPE_BRIDGE:
return iface->data.bridge.virtPortProfile;
case VIR_DOMAIN_NET_TYPE_NETWORK:
if (!iface->data.network.actual)
return NULL;
switch (iface->data.network.actual->type) {
case VIR_DOMAIN_NET_TYPE_DIRECT:
return iface->data.network.actual->data.direct.virtPortProfile;
case VIR_DOMAIN_NET_TYPE_BRIDGE:
return iface->data.network.actual->data.bridge.virtPortProfile;
default:
return NULL;
}
default:
return NULL;
}
}
Then use that function instead.
/* 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 0a2795d..b7eb7d1 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 */
@@ -594,6 +595,7 @@ struct _virDomainActualNetDef {
union {
struct {
char *brname;
+ virNetDevOpenvswitchPortPtr ovsPort;
replace with
virNetDevVPortProfilePtr virtPortProfile;
} bridge;
struct {
char *linkdev;
@@ -645,6 +647,7 @@ struct _virDomainNetDef {
struct {
char *brname;
char *ipaddr;
+ virNetDevOpenvswitchPortPtr ovsPort;
Same.
} bridge;
struct {
char *name;
@@ -1877,6 +1880,9 @@ virDomainNetGetActualDirectVirtPortProfile(virDomainNetDefPtr iface);
virNetDevBandwidthPtr
virDomainNetGetActualBandwidth(virDomainNetDefPtr iface);
+virNetDevOpenvswitchPortPtr
+virDomainNetGetActualOpenvswitchPortPtr(virDomainNetDefPtr iface);
+
Unnecessary.
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
Again, if you appropriately reuse netdev_vport_profile_conf.[ch] as
outlined above, you will no longer need to add these two new files.
index 0000000..ddae3aa
--- /dev/null
+++ b/src/conf/netdev_openvswitch_conf.c
@@ -0,0 +1,94 @@
+/*
+ * 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@xxxxxxxxxx>
+ * Kyle Mestery<kmestery@xxxxxxxxx>
+ * Ansis Atteka<aatteka@xxxxxxxxxx>
+ */
+
+#include<config.h>
+
+#include "netdev_openvswitch_conf.h"
+#include "virterror_internal.h"
+#include "memory.h"
+#include "uuid.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)
+{
+ char *InterfaceID = NULL;
+ virNetDevOpenvswitchPortPtr ovsPort = NULL;
+ xmlNodePtr cur = node->children;
+
+ if (VIR_ALLOC(ovsPort)< 0) {
+ virReportOOMError();
+ goto error;
+ }
+
+ while (cur != NULL) {
+ if (xmlStrEqual(cur->name, BAD_CAST "parameters")) {
+ InterfaceID = virXMLPropString(cur, "interfaceid");
+ break;
+ }
+ cur = cur->next;
+ }
+
+ if (InterfaceID == NULL || (strlen(InterfaceID) == 0)) {
+ // interfaceID does not have to be a UUID,
+ // but a UUID is a reasonable default
+ if (virUUIDGenerateStr(ovsPort->InterfaceID)) {
+ virNetDevError(VIR_ERR_XML_ERROR, "%s",
+ _("cannot generate a random uuid for interfaceid"));
+ goto error;
+ }
+ } else {
+ if (virStrcpyStatic(ovsPort->InterfaceID, InterfaceID) == NULL) {
+ virNetDevError(VIR_ERR_XML_ERROR, "%s",
+ _("InterfaceID parameter too long"));
+ goto error;
+ }
+ }
+
+cleanup:
+ return ovsPort;
+
+error:
+ VIR_FREE(ovsPort);
+ goto cleanup;
+}
+
+
+int
+virNetDevOpenvswitchPortFormat(virNetDevOpenvswitchPortPtr ovsPort,
+ virBufferPtr buf)
+{
+ if (ovsPort == NULL)
+ return 0;
+
+ virBufferAsprintf(buf, "<virtualport type='openvswitch'>\n");
+ virBufferAsprintf(buf, "<parameters interfaceid='%s'/>\n",
+ ovsPort->InterfaceID);
+ virBufferAddLit(buf, "</virtualport>\n");
+ return 0;
+}
diff --git a/src/conf/netdev_openvswitch_conf.h b/src/conf/netdev_openvswitch_conf.h
new file mode 100644
Same as previous file
index 0000000..fd8e079
--- /dev/null
+++ b/src/conf/netdev_openvswitch_conf.h
@@ -0,0 +1,39 @@
+/*
+ * 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@xxxxxxxxxx>
+ * Kyle Mestery<kmestery@xxxxxxxxx>
+ * Ansis Atteka<aatteka@xxxxxxxxxx>
+ */
+
+#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__ */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d6ad36c..0666d5c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -379,6 +379,7 @@ virDomainNetGetActualBridgeName;
virDomainNetGetActualDirectDev;
virDomainNetGetActualDirectMode;
virDomainNetGetActualDirectVirtPortProfile;
+virDomainNetGetActualOpenvswitchPortPtr;
virDomainNetGetActualType;
virDomainNetIndexByMac;
virDomainNetInsert;
@@ -738,6 +739,10 @@ virShrinkN;
virNetDevBandwidthFormat;
virNetDevBandwidthParse;
+# netdev_openvswitch_conf.h
+virNetDevOpenvswitchPortFormat;
+virNetDevOpenvswitchPortParse;
+
# netdev_vportprofile_conf.h
virNetDevVPortProfileFormat;
@@ -1145,6 +1150,7 @@ virGetHostUUID;
virSetHostUUIDStr;
virUUIDFormat;
virUUIDGenerate;
+virUUIDGenerateStr;
Is there a reason you're allocating this as a string rather than
following the existing convention, and just converting it to a string
when you need to use it? That would make it identical to instanceID in
the 8021Qbg case of the virtualPortProfile (you could actually create
the virportprofile openvswitch case for parse and format by just copying
the 8021Qbg case and removing the extra fields).
virUUIDParse;
@@ -1237,10 +1243,16 @@ virNetDevMacVLanCreateWithVPortProfile;
virNetDevMacVLanDeleteWithVPortProfile;
+# virnetdevopenvswitch.h
+virNetDevOpenvswitchAddPort;
+virNetDevOpenvswitchDelPort;
To match the naming for linux host bridges, this function should be
called virNetDevOpenvswitchRemovePort().
+
+
# virnetdevtap.h
virNetDevTapCreate;
virNetDevTapCreateInBridgePort;
virNetDevTapDelete;
+virNetDevTapDeleteInBridgePort;
Based on what's happening in this function (not much), I've changed my
mind about it, and think that you should instead just be directly
calling virNetDevOpenvswitchRemovePort() when appropriate.
# virnetdevveth.h
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index b712da4..1fab77a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
I notice that you didn't do anything in lxcSetupInterfaceBridged(). It
doesn't call virNetDevTapCreateInBridgePort because it doesn't use tap
devices. Instead, it calls virNetDevBridgeAddPort(). I guess to be
consistent, you should modify that call to instead be:
virNetDevVPortProfilePtr vport =
virDomainNetGetActualVirtPortProfile(net);
if (vport && vport->virtPortType ==
VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
ret = virNetDevOpenvswitchAddPort(brname, parentVeth, vport);
else
ret = virNetDevBridgeAddPort(brname, parentVeth);
if (ret < 0)
goto cleanup;
@@ -56,6 +56,7 @@
#include "domain_nwfilter.h"
#include "network/bridge_driver.h"
#include "virnetdev.h"
+#include "virnetdevtap.h"
#include "virnodesuspend.h"
#include "virtime.h"
#include "virtypedparam.h"
@@ -1132,10 +1133,12 @@ static void lxcVmCleanup(lxc_driver_t *driver,
priv->monitorWatch = -1;
for (i = 0 ; i< vm->def->nnets ; i++) {
- ignore_value(virNetDevSetOnline(vm->def->nets[i]->ifname, false));
- ignore_value(virNetDevVethDelete(vm->def->nets[i]->ifname));
-
- networkReleaseActualDevice(vm->def->nets[i]);
+ virDomainNetDefPtr iface = vm->def->nets[i];
+ ignore_value(virNetDevSetOnline(iface->ifname, false));
+ ignore_value(virNetDevVethDelete(iface->ifname));
+ ignore_value(virNetDevTapDeleteInBridgePort(iface->ifname,
+ virDomainNetGetActualOpenvswitchPortPtr(iface)));
Again, since lxc doesn't use tap devices, it couldn't use this
higher-level function (even if we decide to keep it). Instead, you'll
want to do:
virNetDevVPortProfilePtr vport =
virDomainNetGetActualVirtPortProfile(iface);
if (vport && vport->virtPortType ==
VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
ignore_value(virNetDevOpenvswitchRemovePort(brname, parentVeth));
(It almost (but not quite yet) seems like it's worth a helper function
virDomainNetGetActualVirtPortProfileType())
Also, although you say it's safe to remove the interface from the switch
after deleting it, I would feel better if you moved virNetDevVethDelete
to be *after* this code :-)
+ networkReleaseActualDevice(iface);
}
virDomainConfVMNWFilterTeardown(vm);
@@ -1377,8 +1380,12 @@ static int lxcSetupInterfaces(virConnectPtr conn,
cleanup:
if (ret != 0) {
- for (i = 0 ; i< def->nnets ; i++)
- networkReleaseActualDevice(def->nets[i]);
+ for (i = 0 ; i< def->nnets ; i++) {
+ virDomainNetDefPtr iface = def->nets[i];
+ ignore_value(virNetDevTapDeleteInBridgePort(iface->ifname,
+ virDomainNetGetActualOpenvswitchPortPtr(iface)));
Again, you should call virNetDevOpenvswitchRemovePort as in the example
above. (As a matter of fact, this cleanup code looks incomplete to me -
I'm not really familiar with the lxc driver, but I think it should also
be virNetDevVethDelete too (that's a pre-existing problem, though, and
it should be fixed in a separate patch).
+ networkReleaseActualDevice(iface);
+ }
}
return ret;
}
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 57ebb9f..1423d3f 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1765,7 +1765,8 @@ 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 0e26df1..d6a81f1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -247,7 +247,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */
BTW, you notice that we give the tap device a different MAC address than
QEMU is giving to the guest, right? I just want to make sure that
openvswitch doesn't assume that all traffic coming from this port has
the MAC address of the tap device...
err = virNetDevTapCreateInBridgePort(brname,&net->ifname, tapmac,
- vnet_hdr, true,&tapfd);
+ vnet_hdr, true,&tapfd,
+ virDomainNetGetActualOpenvswitchPortPtr(net));
virDomainNetGetActualVirtPortProfile(net)
virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0);
if (err< 0) {
if (template_ifname)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index b4870be..85a004d 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -40,6 +40,7 @@
#include "qemu_cgroup.h"
#include "locking/domain_lock.h"
#include "network/bridge_driver.h"
+#include "virnetdevtap.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -837,7 +838,8 @@ cleanup:
if (iface_connected)
virDomainConfNWFilterTeardown(net);
-
+ ignore_value(virNetDevTapDeleteInBridgePort(net->ifname,
+ virDomainNetGetActualOpenvswitchPortPtr(net)));
Again, since the virNetDevTapDeleteInBridgePort function ends up not
actually doing anything with a tap device, I think this should be
replaced with a direct call to virNetDevOpenvswitchRemovePort (with
appropriate checks).
networkReleaseActualDevice(net);
}
@@ -1937,7 +1939,8 @@ int qemuDomainDetachNetDevice(struct qemud_driver *driver,
detach->ifname);
}
}
-
+ ignore_value(virNetDevTapDeleteInBridgePort(detach->ifname,
+ virDomainNetGetActualOpenvswitchPortPtr(detach)));
Same as above.
networkReleaseActualDevice(detach);
if (vm->def->nnets> 1) {
memmove(vm->def->nets + i,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 2d92d66..14eeef8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -62,6 +62,7 @@
#include "network/bridge_driver.h"
#include "uuid.h"
#include "virtime.h"
+#include "virnetdevtap.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -3731,6 +3732,8 @@ void qemuProcessStop(struct qemud_driver *driver,
/* release the physical device (or any other resources used by
* this interface in the network driver
*/
+ ignore_value(virNetDevTapDeleteInBridgePort(net->ifname,
+ virDomainNetGetActualOpenvswitchPortPtr(net)));
Same as above.
(hopefully all this repetition is going to make it obvious how best to
group together all the net device setup/teardown functions into
general-purpose helpers so that future additions/changes can be in just
a couple places instead of all over).
networkReleaseActualDevice(net);
}
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index 316d558..b8610ea 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -142,7 +142,8 @@ 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,
+ virDomainNetGetActualOpenvswitchPortPtr(net))< 0) {
virDomainNetGetActualVirtPortProfile(net)
if (template_ifname)
VIR_FREE(net->ifname);
goto error;
diff --git a/src/util/uuid.c b/src/util/uuid.c
index 23822ec..d8188b7 100644
--- a/src/util/uuid.c
+++ b/src/util/uuid.c
@@ -116,6 +116,25 @@ virUUIDGenerate(unsigned char *uuid)
}
/**
+ * virUUIDGenerateString:
Again, I don't think this function is necessary - just store the UUID in
binary, and call format as necessary when generating the commandline.
+ * @uuid: string of VIR_UUID_STRING_BUFLEN characters to store the UUID
+ *
+ *
+ * Generates a randomized unique identifier
+ * Returns 0 in case of success and -1 in case of failure
+ */
+int
+virUUIDGenerateStr(char *uuidstr)
+{
+ unsigned char uuid[VIR_UUID_BUFLEN];
+
+ if (virUUIDGenerate(uuid)< 0)
+ return -1;
+ virUUIDFormat(uuid, uuidstr);
+ return 0;
+}
+
+/**
* virUUIDParse:
* @uuidstr: zero terminated string representation of the UUID
* @uuid: array of VIR_UUID_BUFLEN bytes to store the raw UUID
diff --git a/src/util/uuid.h b/src/util/uuid.h
index 7dbfad5..1ae7efb 100644
--- a/src/util/uuid.h
+++ b/src/util/uuid.h
@@ -30,6 +30,7 @@ int virGetHostUUID(unsigned char *host_uuid) ATTRIBUTE_NONNULL(1);
int virUUIDIsValid(unsigned char *uuid);
int virUUIDGenerate(unsigned char *uuid);
+int virUUIDGenerateStr(char *struuid);
int virUUIDParse(const char *uuidstr,
unsigned char *uuid)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
new file mode 100644
index 0000000..c5bef8d
--- /dev/null
+++ b/src/util/virnetdevopenvswitch.c
@@ -0,0 +1,114 @@
+/*
+ * 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@xxxxxxxxxx>
+ * Kyle Mestery<kmestery@xxxxxxxxx>
+ * Ansis Atteka<aatteka@xxxxxxxxxx>
+ */
+
+#include<config.h>
+
+#include "virnetdevopenvswitch.h"
+#include "command.h"
+#include "memory.h"
+#include "virterror_internal.h"
+#include "ignore-value.h"
+#include "virmacaddr.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
+ *
+ * Add an interface to the OVS bridge
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname,
+ const unsigned char *macaddr,
+ virNetDevOpenvswitchPortPtr ovsport)
virNetDevVPortProfilePtr vport)
{
+ int ret = -1;
+ virCommandPtr cmd = NULL;
+ char macaddrstr[VIR_MAC_STRING_BUFLEN];
+ char *attachedmac_ex_id = NULL;
+ char *ifaceid_ex_id = NULL;
+
+ virMacAddrFormat(macaddr, macaddrstr);
+
+ if (virAsprintf(&attachedmac_ex_id, "external-ids:attached-mac=\"%s\"",
+ macaddrstr)< 0)
+ goto cleanup;
+ if (virAsprintf(&ifaceid_ex_id, "external-ids:iface-id=\"%s\"",
+ ovsport->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);
+ goto cleanup;
+ }
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(attachedmac_ex_id);
+ VIR_FREE(ifaceid_ex_id);
+ virCommandFree(cmd);
+ return ret;
+}
+
+/**
+ * virNetDevOpenvswitchDelPort:
+ * @ifname: the network interface name
+ *
+ * Deletes an interface from a OVS bridge
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int virNetDevOpenvswitchDelPort(const char *ifname)
For consistency with the corresponding host bridge function, this should be
int virNetDevBridgeRemovePort(const char *brname ATTRIBUTE_UNUSED,
const char *ifname)
(even though brname is apparently unused) - someday we may want to have
this function called from a function pointer for some reason.
+{
+ int ret = -1;
+ virCommandPtr cmd = NULL;
+
+ cmd = virCommandNew(OVSVSCTL);
+ virCommandAddArgList(cmd, "--", "--if-exists", "del-port", ifname, NULL);
+
+ if (virCommandRun(cmd, NULL)< 0) {
+ virReportSystemError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to delete port %s from OVS"), ifname);
+ goto cleanup;
+ }
+ ret = 0;
+
+ cleanup:
+ virCommandFree(cmd);
+ return ret;
+}
diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
new file mode 100644
index 0000000..210546b
--- /dev/null
+++ b/src/util/virnetdevopenvswitch.h
@@ -0,0 +1,46 @@
+/*
+ * 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@xxxxxxxxxx>
+ * Kyle Mestery<kmestery@xxxxxxxxx>
+ * Ansis Atteka<aatteka@xxxxxxxxxx>
+ */
+
+#ifndef __VIR_NETDEV_OPENVSWITCH_H__
+# define __VIR_NETDEV_OPENVSWITCH_H__
+
+# include "internal.h"
+# include "util.h"
+
+typedef struct _virNetDevOpenvswitchPort virNetDevOpenvswitchPort;
+typedef virNetDevOpenvswitchPort *virNetDevOpenvswitchPortPtr;
+struct _virNetDevOpenvswitchPort {
+ char InterfaceID[VIR_UUID_STRING_BUFLEN];
+};
+
+int virNetDevOpenvswitchAddPort(const char *brname,
+ const char *ifname,
+ const unsigned char *macaddr,
+ virNetDevOpenvswitchPortPtr ovsport)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
+
+int virNetDevOpenvswitchDelPort(const char *ifname)
+ 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..975bb85 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
+ * @ovsport: Open vSwitch specific configuration
*
* 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,7 +267,8 @@ int virNetDevTapCreateInBridgePort(const char *brname,
const unsigned char *macaddr,
int vnet_hdr,
bool up,
- int *tapfd)
+ int *tapfd,
+ virNetDevOpenvswitchPortPtr ovsport)
virNetDevVPortProfilePtr vport)
{
if (virNetDevTapCreate(ifname, vnet_hdr, tapfd)< 0)
return -1;
@@ -286,8 +289,13 @@ int virNetDevTapCreateInBridgePort(const char *brname,
if (virNetDevSetMTUFromDevice(*ifname, brname)< 0)
goto error;
Does the above function succeed when brname contains the name of an ovs
device?
- if (virNetDevBridgeAddPort(brname, *ifname)< 0)
- goto error;
+ if (ovsport) {
+ if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr, ovsport)< 0)
+ goto error;
+ } else {
+ if (virNetDevBridgeAddPort(brname, *ifname)< 0)
+ goto error;
+ }
I was on the border about putting this in here, since it's bringing
details about openvswitch into what should be a low level function that
doesn't know about much. But on the other hand, this function is by
definition like that, since it's mixing bridge and tap functionality, so
I still think it's okay (others may disagree with me though).
if (virNetDevSetOnline(*ifname, up)< 0)
goto error;
@@ -299,3 +307,21 @@ int virNetDevTapCreateInBridgePort(const char *brname,
return errno;
}
+
+/**
+ * virNetDevTapDeleteInBridgePort:
+ * @ifname: the interface name (or name template)
+ * @ovsport: Open vSwitch specific configuration
+ *
+ * This function detaches tap device from a bridge.
+ *
+ * Returns 0 in case of success or -1 on failure
+ */
+int virNetDevTapDeleteInBridgePort(char *ifname,
+ virNetDevOpenvswitchPortPtr ovsport)
+{
+ int ret = 0;
+ if (ovsport)
+ ret = virNetDevOpenvswitchDelPort(ifname);
+ return ret;
+}
Again, just due to the fact that this doesn't end up actually doing
anything to the tap device, I changed my mind and now think it should be
eliminated in favor of directly calling virNetDevOpenvswitchRemovePort()
directly, when appropriate.
diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index fb35ac5..5b16570 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"
I think I recall being told that, while it's okay for something in the
conf directory to include/use something in the util directory, the
reverse is not true. Fortunately, this will be a non-issue if you're
using virNetDevVPortProfilePtr instead, since it's defined in
util/virnetdevvportprofile.h.
int virNetDevTapCreate(char **ifname,
int vnet_hdr,
@@ -38,8 +39,13 @@ int virNetDevTapCreateInBridgePort(const char *brname,
const unsigned char *macaddr,
int vnet_hdr,
bool up,
- int *tapfd)
+ int *tapfd,
+ virNetDevOpenvswitchPortPtr ovsport)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_RETURN_CHECK;
+int virNetDevTapDeleteInBridgePort(char *ifname,
+ virNetDevOpenvswitchPortPtr ovsport)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+
#endif /* __VIR_NETDEV_TAP_H__ */
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list