[libvirt] [PATCH v2 0/2] network: Bring netdevs online later
The following patchset introduces code to defer setting netdevs online (and therefore registering MACs) until right before beginning guest CPU execution. The first patch introduces some infrastructure changes in preparation of the actual function added in the 2nd patch. Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461 Changes for v2: * Ping for comments, esp. on patch #2. * Moved @flags operand of virNetDevMacVLanCreateWithVPortProfile to the end of the operand list. * Minor changes based on comments by Martin Kletzander. * Added detail to patch #2 commit message. * Martin suggested that I replace various ? operations with if/else statements, but I ended up leaving them alone as they were being used to conditionally assign values to constant fields. * I left the contents of qemu_interface as-is, rather than collapsing them into non-qemu-specific functions, in order to keep Makefile linkage consistent & happy (needs to be part of QEMU_DRIVER_SOURCES). Instead, incorporated copyright suggestions from previous comments. Martin, if you feel strongly about not having these new functions in a qemu-specific part, feel free to comment. Changes since RFC: * Add a separate patch to introduce a flags field for macvlan/macvtap creation. * Use macvlan/tap IFUP flags to skip virNetDevSetOnline (for qemu only). * Add hotplug support. * For macvlan, save the current virNetDevVPortProfileOp in virDomainNetDef during qemuPhysIfaceConnect. As Laine mentioned, could use this field in a future patch to eliminate passing virNetDevVPortProfileOp everywhere. * Add qemu_interface.c and qemu_interface.h to encapsulate new functions. Matthew Rosato (2): util: Introduce flags field for macvtap creation network: Bring netdevs online later src/Makefile.am | 3 +- src/conf/domain_conf.h | 2 ++ src/lxc/lxc_process.c | 6 ++-- src/qemu/qemu_command.c | 12 +++++-- 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 | 36 ++++++++++++-------- src/util/virnetdevmacvlan.h | 16 ++++++--- 10 files changed, 172 insertions(+), 24 deletions(-) create mode 100644 src/qemu/qemu_interface.c create mode 100644 src/qemu/qemu_interface.h -- 1.7.9.5
Currently, there is one flag passed in during macvtap creation (withTap) -- Let's convert this field to an unsigned int flag field for future expansion. Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> --- src/lxc/lxc_process.c | 4 ++-- src/qemu/qemu_command.c | 6 ++++-- src/util/virnetdevmacvlan.c | 28 +++++++++++++++++----------- src/util/virnetdevmacvlan.h | 14 ++++++++++---- 4 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 3353dc1..ed30c37 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -331,12 +331,12 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), virDomainNetGetActualDirectMode(net), - false, false, def->uuid, + false, def->uuid, virDomainNetGetActualVirtPortProfile(net), &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net)) < 0) + virDomainNetGetActualBandwidth(net), 0) < 0) goto cleanup; ret = res_ifname; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9241f57..1f71fa3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -177,6 +177,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def, char *res_ifname = NULL; int vnet_hdr = 0; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) && net->model && STREQ(net->model, "virtio")) @@ -186,11 +187,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), virDomainNetGetActualDirectMode(net), - true, vnet_hdr, def->uuid, + vnet_hdr, def->uuid, virDomainNetGetActualVirtPortProfile(net), &res_ifname, vmop, cfg->stateDir, - virDomainNetGetActualBandwidth(net)); + virDomainNetGetActualBandwidth(net), + macvlan_create_flags); if (rc >= 0) { virDomainAuditNetDevice(def, net, res_ifname, true); VIR_FREE(net->ifname); diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 054194b..9ddeca4 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * @macaddress: The MAC address for the macvtap device * @linkdev: The interface name of the NIC to connect to the external bridge * @mode: int describing the mode for 'bridge', 'vepa', 'private' or 'passthru'. + * @flags: OR of virNetDevMacVLanCreateFlags. * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it * @vmuuid: The UUID of the VM the macvtap belongs to * @virtPortProfile: pointer to object holding the virtual port profile data @@ -797,25 +798,29 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * interface will be stored into if everything succeeded. It is up * to the caller to free the string. * - * Returns file descriptor of the tap device in case of success with @withTap, - * otherwise returns 0; returns -1 on error. + * Returns file descriptor of the tap device in case of success with + * @flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns + * -1 on error. */ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, const virMacAddr *macaddress, const char *linkdev, virNetDevMacVLanMode mode, - bool withTap, int vnet_hdr, const unsigned char *vmuuid, virNetDevVPortProfilePtr virtPortProfile, char **res_ifname, virNetDevVPortProfileOp vmOp, char *stateDir, - virNetDevBandwidthPtr bandwidth) + virNetDevBandwidthPtr bandwidth, + unsigned int flags) { - const char *type = withTap ? "macvtap" : "macvlan"; - const char *prefix = withTap ? MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; - const char *pattern = withTap ? MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; + const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + "macvtap" : "macvlan"; + const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; + const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; int c, rc; char ifname[IFNAMSIZ]; int retries, do_retry = 0; @@ -903,7 +908,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto disassociate_exit; } - if (withTap) { + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0) goto disassociate_exit; @@ -922,7 +927,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, } if (virNetDevBandwidthSet(cr_ifname, bandwidth, false) < 0) { - if (withTap) + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ else rc = -1; @@ -1066,15 +1071,16 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname ATTRIBUTE_UNUSED, const virMacAddr *macaddress ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, virNetDevMacVLanMode mode ATTRIBUTE_UNUSED, - bool withTap ATTRIBUTE_UNUSED, int vnet_hdr ATTRIBUTE_UNUSED, const unsigned char *vmuuid ATTRIBUTE_UNUSED, virNetDevVPortProfilePtr virtPortProfile ATTRIBUTE_UNUSED, char **res_ifname ATTRIBUTE_UNUSED, virNetDevVPortProfileOp vmop ATTRIBUTE_UNUSED, char *stateDir ATTRIBUTE_UNUSED, - virNetDevBandwidthPtr bandwidth ATTRIBUTE_UNUSED) + virNetDevBandwidthPtr bandwidth ATTRIBUTE_UNUSED, + unsigned int flags) { + virCheckFlags(0, NULL); virReportSystemError(ENOSYS, "%s", _("Cannot create macvlan devices on this platform")); return -1; diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 9b15a31..e92da4a 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -40,6 +40,12 @@ typedef enum { } virNetDevMacVLanMode; VIR_ENUM_DECL(virNetDevMacVLanMode) +typedef enum { + VIR_NETDEV_MACVLAN_CREATE_NONE = 0, + /* Create with a tap device */ + VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, +} virNetDevMacVLanCreateFlags; + int virNetDevMacVLanCreate(const char *ifname, const char *type, const virMacAddr *macaddress, @@ -56,16 +62,16 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname, const virMacAddr *macaddress, const char *linkdev, virNetDevMacVLanMode mode, - bool withTap, int vnet_hdr, const unsigned char *vmuuid, virNetDevVPortProfilePtr virtPortProfile, char **res_ifname, virNetDevVPortProfileOp vmop, char *stateDir, - virNetDevBandwidthPtr bandwidth) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(7) - ATTRIBUTE_NONNULL(9) ATTRIBUTE_NONNULL(11) ATTRIBUTE_RETURN_CHECK; + virNetDevBandwidthPtr bandwidth, + unsigned int flags) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6) + ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(10) ATTRIBUTE_RETURN_CHECK; int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, const virMacAddr *macaddress, -- 1.7.9.5
On Wed, Aug 27, 2014 at 10:34:13AM -0400, Matthew Rosato wrote:
Currently, there is one flag passed in during macvtap creation (withTap) -- Let's convert this field to an unsigned int flag field for future expansion.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> --- src/lxc/lxc_process.c | 4 ++-- src/qemu/qemu_command.c | 6 ++++-- src/util/virnetdevmacvlan.c | 28 +++++++++++++++++----------- src/util/virnetdevmacvlan.h | 14 ++++++++++---- 4 files changed, 33 insertions(+), 19 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 3353dc1..ed30c37 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -331,12 +331,12 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), virDomainNetGetActualDirectMode(net), - false, false, def->uuid, + false, def->uuid, virDomainNetGetActualVirtPortProfile(net), &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net)) < 0) + virDomainNetGetActualBandwidth(net), 0) < 0) goto cleanup;
ret = res_ifname; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9241f57..1f71fa3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -177,6 +177,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def, char *res_ifname = NULL; int vnet_hdr = 0; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) && net->model && STREQ(net->model, "virtio")) @@ -186,11 +187,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), virDomainNetGetActualDirectMode(net), - true, vnet_hdr, def->uuid, + vnet_hdr, def->uuid, virDomainNetGetActualVirtPortProfile(net), &res_ifname, vmop, cfg->stateDir, - virDomainNetGetActualBandwidth(net)); + virDomainNetGetActualBandwidth(net), + macvlan_create_flags); if (rc >= 0) { virDomainAuditNetDevice(def, net, res_ifname, true); VIR_FREE(net->ifname); diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 054194b..9ddeca4 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * @macaddress: The MAC address for the macvtap device * @linkdev: The interface name of the NIC to connect to the external bridge * @mode: int describing the mode for 'bridge', 'vepa', 'private' or 'passthru'. + * @flags: OR of virNetDevMacVLanCreateFlags.
I took the liberty of moving this as a last parameter as well.
* @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it * @vmuuid: The UUID of the VM the macvtap belongs to * @virtPortProfile: pointer to object holding the virtual port profile data @@ -797,25 +798,29 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * interface will be stored into if everything succeeded. It is up * to the caller to free the string. * - * Returns file descriptor of the tap device in case of success with @withTap, - * otherwise returns 0; returns -1 on error. + * Returns file descriptor of the tap device in case of success with + * @flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns + * -1 on error. */ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, const virMacAddr *macaddress, const char *linkdev, virNetDevMacVLanMode mode, - bool withTap, int vnet_hdr, const unsigned char *vmuuid, virNetDevVPortProfilePtr virtPortProfile, char **res_ifname, virNetDevVPortProfileOp vmOp, char *stateDir, - virNetDevBandwidthPtr bandwidth) + virNetDevBandwidthPtr bandwidth, + unsigned int flags) { - const char *type = withTap ? "macvtap" : "macvlan"; - const char *prefix = withTap ? MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; - const char *pattern = withTap ? MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; + const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + "macvtap" : "macvlan"; + const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; + const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; int c, rc; char ifname[IFNAMSIZ]; int retries, do_retry = 0; @@ -903,7 +908,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto disassociate_exit; }
- if (withTap) { + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0) goto disassociate_exit;
@@ -922,7 +927,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, }
if (virNetDevBandwidthSet(cr_ifname, bandwidth, false) < 0) { - if (withTap) + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ else rc = -1; @@ -1066,15 +1071,16 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname ATTRIBUTE_UNUSED, const virMacAddr *macaddress ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, virNetDevMacVLanMode mode ATTRIBUTE_UNUSED, - bool withTap ATTRIBUTE_UNUSED, int vnet_hdr ATTRIBUTE_UNUSED, const unsigned char *vmuuid ATTRIBUTE_UNUSED, virNetDevVPortProfilePtr virtPortProfile ATTRIBUTE_UNUSED, char **res_ifname ATTRIBUTE_UNUSED, virNetDevVPortProfileOp vmop ATTRIBUTE_UNUSED, char *stateDir ATTRIBUTE_UNUSED, - virNetDevBandwidthPtr bandwidth ATTRIBUTE_UNUSED) + virNetDevBandwidthPtr bandwidth ATTRIBUTE_UNUSED, + unsigned int flags) { + virCheckFlags(0, NULL);
The flag check could be fixed not to report in such cases, but it's easier just to check the flags for now. But it should be: virCheckFlags(0, -1);
virReportSystemError(ENOSYS, "%s", _("Cannot create macvlan devices on this platform")); return -1; diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 9b15a31..e92da4a 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -40,6 +40,12 @@ typedef enum { } virNetDevMacVLanMode; VIR_ENUM_DECL(virNetDevMacVLanMode)
+typedef enum { + VIR_NETDEV_MACVLAN_CREATE_NONE = 0, + /* Create with a tap device */ + VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0,
You aligned only one value. Unless you object to it, I'll align them together without too many whitespaces. Other than that, it looks fine, ACK after 1.2.8, I'll push it with the changes mentioned after the release if you're OK with that. Martin
On 08/28/2014 08:45 AM, Martin Kletzander wrote:
On Wed, Aug 27, 2014 at 10:34:13AM -0400, Matthew Rosato wrote:
Currently, there is one flag passed in during macvtap creation (withTap) -- Let's convert this field to an unsigned int flag field for future expansion.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> --- src/lxc/lxc_process.c | 4 ++-- src/qemu/qemu_command.c | 6 ++++-- src/util/virnetdevmacvlan.c | 28 +++++++++++++++++----------- src/util/virnetdevmacvlan.h | 14 ++++++++++---- 4 files changed, 33 insertions(+), 19 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 3353dc1..ed30c37 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -331,12 +331,12 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), virDomainNetGetActualDirectMode(net), - false, false, def->uuid, + false, def->uuid, virDomainNetGetActualVirtPortProfile(net), &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net)) < 0) + virDomainNetGetActualBandwidth(net), 0) < 0) goto cleanup;
ret = res_ifname; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9241f57..1f71fa3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -177,6 +177,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def, char *res_ifname = NULL; int vnet_hdr = 0; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) && net->model && STREQ(net->model, "virtio")) @@ -186,11 +187,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname, &net->mac, virDomainNetGetActualDirectDev(net), virDomainNetGetActualDirectMode(net), - true, vnet_hdr, def->uuid, + vnet_hdr, def->uuid, virDomainNetGetActualVirtPortProfile(net), &res_ifname, vmop, cfg->stateDir, - virDomainNetGetActualBandwidth(net)); + virDomainNetGetActualBandwidth(net), + macvlan_create_flags); if (rc >= 0) { virDomainAuditNetDevice(def, net, res_ifname, true); VIR_FREE(net->ifname); diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 054194b..9ddeca4 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * @macaddress: The MAC address for the macvtap device * @linkdev: The interface name of the NIC to connect to the external bridge * @mode: int describing the mode for 'bridge', 'vepa', 'private' or 'passthru'. + * @flags: OR of virNetDevMacVLanCreateFlags.
I took the liberty of moving this as a last parameter as well.
* @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it * @vmuuid: The UUID of the VM the macvtap belongs to * @virtPortProfile: pointer to object holding the virtual port profile data @@ -797,25 +798,29 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * interface will be stored into if everything succeeded. It is up * to the caller to free the string. * - * Returns file descriptor of the tap device in case of success with @withTap, - * otherwise returns 0; returns -1 on error. + * Returns file descriptor of the tap device in case of success with + * @flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns + * -1 on error. */ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, const virMacAddr *macaddress, const char *linkdev, virNetDevMacVLanMode mode, - bool withTap, int vnet_hdr, const unsigned char *vmuuid, virNetDevVPortProfilePtr virtPortProfile, char **res_ifname, virNetDevVPortProfileOp vmOp, char *stateDir, - virNetDevBandwidthPtr bandwidth) + virNetDevBandwidthPtr bandwidth, + unsigned int flags) { - const char *type = withTap ? "macvtap" : "macvlan"; - const char *prefix = withTap ? MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; - const char *pattern = withTap ? MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; + const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + "macvtap" : "macvlan"; + const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; + const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; int c, rc; char ifname[IFNAMSIZ]; int retries, do_retry = 0; @@ -903,7 +908,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto disassociate_exit; }
- if (withTap) { + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0) goto disassociate_exit;
@@ -922,7 +927,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, }
if (virNetDevBandwidthSet(cr_ifname, bandwidth, false) < 0) { - if (withTap) + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ else rc = -1; @@ -1066,15 +1071,16 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname ATTRIBUTE_UNUSED, const virMacAddr *macaddress ATTRIBUTE_UNUSED, const char *linkdev ATTRIBUTE_UNUSED, virNetDevMacVLanMode mode ATTRIBUTE_UNUSED, - bool withTap ATTRIBUTE_UNUSED, int vnet_hdr ATTRIBUTE_UNUSED, const unsigned char *vmuuid ATTRIBUTE_UNUSED, virNetDevVPortProfilePtr virtPortProfile ATTRIBUTE_UNUSED, char **res_ifname ATTRIBUTE_UNUSED, virNetDevVPortProfileOp vmop ATTRIBUTE_UNUSED, char *stateDir ATTRIBUTE_UNUSED, - virNetDevBandwidthPtr bandwidth ATTRIBUTE_UNUSED) + virNetDevBandwidthPtr bandwidth ATTRIBUTE_UNUSED, + unsigned int flags) { + virCheckFlags(0, NULL);
The flag check could be fixed not to report in such cases, but it's easier just to check the flags for now. But it should be:
virCheckFlags(0, -1);
virReportSystemError(ENOSYS, "%s", _("Cannot create macvlan devices on this platform")); return -1; diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 9b15a31..e92da4a 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -40,6 +40,12 @@ typedef enum { } virNetDevMacVLanMode; VIR_ENUM_DECL(virNetDevMacVLanMode)
+typedef enum { + VIR_NETDEV_MACVLAN_CREATE_NONE = 0, + /* Create with a tap device */ + VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0,
You aligned only one value. Unless you object to it, I'll align them together without too many whitespaces.
Other than that, it looks fine, ACK after 1.2.8, I'll push it with the changes mentioned after the release if you're OK with that.
Yep, I'm fine with the changes you've mentioned. Thanks Martin!
Martin
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@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 \ + qemu/qemu_interface.c qemu/qemu_interface.h 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; 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) { 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@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@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); + 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
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@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@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@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
On 08/28/2014 09:56 AM, Martin Kletzander wrote:
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@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.
I mentioned this in the cover letter comments, but you may have missed it: " * I left the contents of qemu_interface as-is, rather than collapsing them into non-qemu-specific functions, in order to keep Makefile linkage consistent & happy (needs to be part of QEMU_DRIVER_SOURCES). Instead, incorporated copyright suggestions from previous comments. " Basically, you're right in that there is nothing inherently qemu-specific in this code, just that it's currently only being written with qemu in mind, and it sticks with the current Makefile convention (everything in QEMU_DRIVER_SOURCES is part of src/qemu/qemu_*).
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.
I thought I had a good reason for doing this -- But I'm not finding it. So, yeah, I think this should be done above so that it affects both paths.
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?
Based on my comment above, I think we also need to be calling for VIR_DOMAIN_NET_TYPE_NETWORK and VIR_DOMAIN_NET_TYPE_BRIDGE then. I guess my rationale was that we would always call the function, and the switch in qemuInterfaceStartDevice() would decide whether or not work needed to be done, minimizing the amount of effort needed to add new devices later. If you'd rather I only call qemuinterfaceStartDevice for the appropriate values of newType, I can certainly do that.
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@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@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.).
Hmm, that's a fair point. I might be able to key off of the 'virDomainRunningReason reason' to decide whether or not the call is necessary. Alternatively, I guess I could just call qemuInterfaceStartDevices() prior to each appropriate qemuProcessStartCPUs call.
Other than these few things the patch looks fine, but I'd rather have another opinion in this code area.
Fair enough. Thanks for the comments. Matt
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
participants (2)
-
Martin Kletzander -
Matthew Rosato