[libvirt] [PATCH v2 2/2] network: Bring netdevs online later
Martin Kletzander
mkletzan at redhat.com
Thu Aug 28 13:56:42 UTC 2014
On Wed, Aug 27, 2014 at 10:34:14AM -0400, Matthew Rosato wrote:
>Currently, MAC registration occurs during device creation, which is
>early enough that, during live migration, you end up with duplicate
>MAC addresses on still-running source and target devices, even though
>the target device isn't actually being used yet.
>This patch proposes to defer MAC registration until right before
>the guest can actually use the device -- In other words, right
>before starting guest CPUs.
>
>Signed-off-by: Matthew Rosato <mjrosato at linux.vnet.ibm.com>
>---
> src/Makefile.am | 3 +-
> src/conf/domain_conf.h | 2 ++
> src/lxc/lxc_process.c | 4 ++-
> src/qemu/qemu_command.c | 6 +++-
> src/qemu/qemu_hotplug.c | 7 ++++
> src/qemu/qemu_interface.c | 78 +++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_interface.h | 32 ++++++++++++++++++
> src/qemu/qemu_process.c | 4 +++
> src/util/virnetdevmacvlan.c | 8 +++--
> src/util/virnetdevmacvlan.h | 2 ++
> 10 files changed, 140 insertions(+), 6 deletions(-)
> create mode 100644 src/qemu/qemu_interface.c
> create mode 100644 src/qemu/qemu_interface.h
>
>diff --git a/src/Makefile.am b/src/Makefile.am
>index 46e411e..842573f 100644
>--- a/src/Makefile.am
>+++ b/src/Makefile.am
>@@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES = \
> qemu/qemu_monitor_text.h \
> qemu/qemu_monitor_json.c \
> qemu/qemu_monitor_json.h \
>- qemu/qemu_driver.c qemu/qemu_driver.h
>+ qemu/qemu_driver.c qemu/qemu_driver.h \
I don't know if this got mixed by the mail client or not, but I'm
adjusting it (the backslash) to match the others.
>+ qemu/qemu_interface.c qemu/qemu_interface.h
>
I still don't see anything qemu-specific inthose files, but for now
separate files are fine, we can move the code around any time later if
it's needed.
> XENAPI_DRIVER_SOURCES = \
> xenapi/xenapi_driver.c xenapi/xenapi_driver.h \
>diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>index aead903..6268690 100644
>--- a/src/conf/domain_conf.h
>+++ b/src/conf/domain_conf.h
>@@ -950,6 +950,8 @@ struct _virDomainNetDef {
> virNetDevBandwidthPtr bandwidth;
> virNetDevVlan vlan;
> int linkstate;
>+ /* vmOp value saved if deferring interface start */
>+ virNetDevVPortProfileOp vmOp;
> };
>
> /* Used for prefix of ifname of any network name generated dynamically
>diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>index ed30c37..b2256c0 100644
>--- a/src/lxc/lxc_process.c
>+++ b/src/lxc/lxc_process.c
>@@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
> virNetDevBandwidthPtr bw;
> virNetDevVPortProfilePtr prof;
> virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
>+ unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
>
> /* XXX how todo bandwidth controls ?
> * Since the 'net-ifname' is about to be moved to a different
>@@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
> &res_ifname,
> VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
> cfg->stateDir,
>- virDomainNetGetActualBandwidth(net), 0) < 0)
>+ virDomainNetGetActualBandwidth(net),
>+ macvlan_create_flags) < 0)
> goto cleanup;
>
> ret = res_ifname;
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index 1f71fa3..43a8b63 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
> net->ifname = res_ifname;
> }
>
>+ /* Save vport profile op for later */
>+ net->vmOp = vmop;
>+
> virObjectUnref(cfg);
> return rc;
> }
>@@ -286,7 +289,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> {
> char *brname = NULL;
> int ret = -1;
>- unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
>+ unsigned int tap_create_flags = 0;
> bool template_ifname = false;
> int actualType = virDomainNetGetActualType(net);
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>@@ -346,6 +349,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> goto cleanup;
> }
> } else {
>+ tap_create_flags |= VIR_NETDEV_TAP_CREATE_IFUP;
But I don't quite understand why this is different for system and
session daemon. Maybe it's my head just giving up on me.
> if (qemuCreateInBridgePortWithHelper(cfg, brname,
> &net->ifname,
> tapfd, tap_create_flags) < 0) {
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index a364c52..633eda5 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -30,6 +30,7 @@
> #include "qemu_domain.h"
> #include "qemu_command.h"
> #include "qemu_hostdev.h"
>+#include "qemu_interface.h"
> #include "domain_audit.h"
> #include "domain_nwfilter.h"
> #include "virlog.h"
>@@ -878,6 +879,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
> if (networkAllocateActualDevice(vm->def, net) < 0)
> goto cleanup;
>
>+ /* Set device online immediately */
>+ qemuInterfaceStartDevice(net);
>+
> actualType = virDomainNetGetActualType(net);
>
> if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>@@ -2069,6 +2073,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
>+ /* Set device online immediately */
>+ qemuInterfaceStartDevice(newdev);
>+
> newType = virDomainNetGetActualType(newdev);
>
> if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
But these two hunks don't make sense to me. Actually they would make
sense under the (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) condition.
Is there any reason why you are making these calls unconditionally?
>diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
>new file mode 100644
>index 0000000..dccfcc4
>--- /dev/null
>+++ b/src/qemu/qemu_interface.c
>@@ -0,0 +1,78 @@
>+/*
>+ * qemu_interface.c: QEMU interface management
>+ *
>+ * Copyright IBM Corp. 2014
>+ *
>+ * 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:
>+ * Matthew J. Rosato <mjrosato at linux.vnet.ibm.com>
>+ */
>+
>+#include <config.h>
>+
>+#include "qemu_interface.h"
>+#include "virnetdev.h"
>+#include "virnetdevtap.h"
>+#include "virnetdevmacvlan.h"
>+#include "virnetdevvportprofile.h"
>+
>+/**
>+ * qemuInterfaceStartDevice:
>+ * @net: net device to start
>+ *
>+ * Based upon the type of device provided, perform the appropriate
>+ * work to set the device online.
>+ */
>+void
>+qemuInterfaceStartDevice(virDomainNetDefPtr net)
>+{
>+ switch (virDomainNetGetActualType(net)) {
>+ case VIR_DOMAIN_NET_TYPE_BRIDGE:
>+ case VIR_DOMAIN_NET_TYPE_NETWORK:
>+ if (virNetDevSetOnline(net->ifname, true) < 0) {
>+ ignore_value(virNetDevTapDelete(net->ifname));
>+ }
>+ break;
>+ case VIR_DOMAIN_NET_TYPE_DIRECT:
>+ if (virNetDevSetOnline(net->ifname, true) < 0) {
>+ ignore_value(virNetDevVPortProfileDisassociate(net->ifname,
>+ virDomainNetGetActualVirtPortProfile(net),
>+ &net->mac,
>+ virDomainNetGetActualDirectDev(net),
>+ -1,
>+ net->vmOp));
>+ }
>+ break;
>+ }
>+}
>+
>+/**
>+ * qemuInterfaceStartDevices:
>+ * @def: domain definition
>+ *
>+ * Set all ifaces associated with this domain to the online state.
>+ */
>+void
>+qemuInterfaceStartDevices(virDomainDefPtr def)
>+{
>+ size_t i;
>+
>+ for (i = 0; i < def->nnets; i++) {
>+ qemuInterfaceStartDevice(def->nets[i]);
>+ }
>+
>+ return;
>+}
>diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h
>new file mode 100644
>index 0000000..5810cda
>--- /dev/null
>+++ b/src/qemu/qemu_interface.h
>@@ -0,0 +1,32 @@
>+/*
>+ * qemu_interface.h: QEMU interface management
>+ *
>+ * Copyright IBM Corp. 2014
>+ *
>+ * 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:
>+ * Matthew J. Rosato <mjrosato at linux.vnet.ibm.com>
>+ */
>+
>+#ifndef __QEMU_INTERFACE_H__
>+# define __QEMU_INTERFACE_H__
>+
>+# include "domain_conf.h"
>+
>+void qemuInterfaceStartDevice(virDomainNetDefPtr net);
>+void qemuInterfaceStartDevices(virDomainDefPtr def);
>+
>+#endif /* __QEMU_INTERFACE_H__ */
>diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>index f68dfbe..edb2b1f 100644
>--- a/src/qemu/qemu_process.c
>+++ b/src/qemu/qemu_process.c
>@@ -42,6 +42,7 @@
> #include "qemu_hostdev.h"
> #include "qemu_hotplug.h"
> #include "qemu_migration.h"
>+#include "qemu_interface.h"
>
> #include "cpu/cpu.h"
> #include "datatypes.h"
>@@ -2854,6 +2855,9 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm,
> qemuDomainObjPrivatePtr priv = vm->privateData;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>
>+ /* Bring up netdevs before starting CPUs */
>+ qemuInterfaceStartDevices(vm->def);
>+
This seems to be in a wrong place, I guess. qemuProcessStartCPUs() is
called from *many* places I'm not sure you need it (resume after dump,
etc.).
Other than these few things the patch looks fine, but I'd rather have
another opinion in this code area.
> VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState));
> if (virDomainLockProcessResume(driver->lockManager, cfg->uri,
> vm, priv->lockState) < 0) {
>diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
>index 9ddeca4..4099090 100644
>--- a/src/util/virnetdevmacvlan.c
>+++ b/src/util/virnetdevmacvlan.c
>@@ -903,9 +903,11 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
> goto link_del_exit;
> }
>
>- if (virNetDevSetOnline(cr_ifname, true) < 0) {
>- rc = -1;
>- goto disassociate_exit;
>+ if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP) {
>+ if (virNetDevSetOnline(cr_ifname, true) < 0) {
>+ rc = -1;
>+ goto disassociate_exit;
>+ }
> }
>
> if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) {
>diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
>index e92da4a..43dc71f 100644
>--- a/src/util/virnetdevmacvlan.h
>+++ b/src/util/virnetdevmacvlan.h
>@@ -44,6 +44,8 @@ typedef enum {
> VIR_NETDEV_MACVLAN_CREATE_NONE = 0,
> /* Create with a tap device */
> VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0,
>+ /* Bring the interface up */
>+ VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1,
> } virNetDevMacVLanCreateFlags;
>
> int virNetDevMacVLanCreate(const char *ifname,
>--
>1.7.9.5
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140828/46f0074c/attachment-0001.sig>
More information about the libvir-list
mailing list