On 03/21/2012 09:21 AM, D. Herrendoerfer wrote:
> From: "D. Herrendoerfer" <d.herrendoerfer@xxxxxxxxxxxxxxxxxx>
>
> When libvirtd is restarted, also restart the netlink event
> message callbacks for existing VEPA connections and send
> a message to lldpad for these existing links, so it learns
> the new libvirtd pid.
> This only includes qemu support as a base if this works out
> I'll add others.
>
> Signed-off-by: D. Herrendoerfer <d.herrendoerfer@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 30 ++++++++++
> src/util/virnetdevmacvlan.c | 128 +++++++++++++++++++++++++++++++++---------
> src/util/virnetdevmacvlan.h | 9 +++
> 3 files changed, 139 insertions(+), 28 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 538a419..c93ece3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -412,6 +412,34 @@ cleanup:
> virDomainObjUnlock(vm);
> }
>
> +
> +static void qemuDomainNetsRestart(void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + void *data)
data also should be ATTRIBUTE_UNUSED.
> +{
> + int i;
> + virDomainObjPtr vm = (virDomainObjPtr)payload;
> + virDomainDefPtr def = vm->def;
> +
> + virDomainObjLock(vm);
> +
> + for (i = 0; i < def->nnets; i++) {
> + virDomainNetDefPtr net = def->nets[i];
> + if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT &&
> + net->data.direct.mode == VIR_NETDEV_MACVLAN_MODE_VEPA) {
> + VIR_DEBUG("Device active on %s in VEPA mode. Reassociating.",net->ifname);
> + ignore_value(virNetDevMacVLanRestartWithVPortProfile(net->ifname,
> + net->mac,
> + net->data.direct.linkdev,
> + def->uuid,
> + net->data.direct.virtPortProfile,
> + VIR_NETDEV_VPORT_PROFILE_OP_CREATE));
This will not work when the interface type='network', and the network is
a pool of host interfaces to use in direct mode. It should be changed
like this:
if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT &&
virDomainNetGetActualDirectMode(net) ==
VIR_NETDEV_MACVLAN_MODE_VEPA) {
VIR_DEBUG("Device active on %s in VEPA mode.
Reassociating.",net->ifname);
ignore_value(virNetDevMacVLanRestartWithVPortProfile(net->ifname,
net->mac,
virDomainNetGetActualDirectDev(net),
def->uuid,
virDomainNetGetActualVirtPortProfile(net),
VIR_NETDEV_VPORT_PROFILE_OP_CREATE));
The VirDomainNetGetActual*() helper functions will retrieve the
appropriate value based on whether this interface was specified as
type='network' or type='direct' in the config.
> + }
> + }
> +
> + virDomainObjUnlock(vm);
> +}
> +
> /**
> * qemudStartup:
> *
> @@ -668,6 +696,8 @@ qemudStartup(int privileged) {
> NULL, NULL) < 0)
> goto error;
>
> + virHashForEach(qemu_driver->domains.objs, qemuDomainNetsRestart, NULL);
> +
> conn = virConnectOpen(qemu_driver->privileged ?
> "qemu:///system" :
> "qemu:///session");
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 647679f..eca4b6e 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -769,6 +769,49 @@ virNetDevMacVLanVPortProfileDestroyCallback(int watch ATTRIBUTE_UNUSED,
> virNetlinkCallbackDataFree((virNetlinkCallbackDataPtr)opaque);
> }
>
> +static int
> +virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
> + const unsigned char *macaddress,
> + const char *linkdev,
> + const unsigned char *vmuuid,
> + virNetDevVPortProfilePtr virtPortProfile,
> + enum virNetDevVPortProfileOp vmOp)
> +{
> + virNetlinkCallbackDataPtr calld = NULL;
> +
> + if (virtPortProfile && virNetlinkEventServiceIsRunning()) {
> + if (VIR_ALLOC(calld) < 0)
> + goto memory_error;
> + if ((calld->cr_ifname = strdup(ifname)) == NULL)
> + goto memory_error;
> + if (VIR_ALLOC(calld->virtPortProfile) < 0)
> + goto memory_error;
> + memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
> + if (VIR_ALLOC_N(calld->macaddress, VIR_MAC_BUFLEN) < 0)
> + goto memory_error;
> + memcpy(calld->macaddress, macaddress, VIR_MAC_BUFLEN);
> + if ((calld->linkdev = strdup(linkdev)) == NULL)
> + goto memory_error;
> + if (VIR_ALLOC_N(calld->vmuuid, VIR_UUID_BUFLEN) < 0)
> + goto memory_error;
> + memcpy(calld->vmuuid, vmuuid, VIR_UUID_BUFLEN);
> +
> + calld->vmOp = vmOp;
> +
> + virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
> + virNetDevMacVLanVPortProfileDestroyCallback,
> + calld, macaddress);
Oops. I just noticed a memory leak that was in the existing code, and
has persisted to this new code - it's possible for
virNetlinkEventAddClient to fail, and in that case the calld object will
not be attached anywhere, but since we don't know that
virNetlinkEventAddClient failed, we don't free it either.
We need to check for the return value of this function, and if it's < 0,
goto error (add an error: label just above
virNetlinkCallbackDataFree(calld)).
Additionally, I see that in the review of the original patches, we
didn't catch that there are error paths in virNetlinkCallbackDataFree()
that don't log any error message. This should also be fixed.
> + }
> +
> + return 0;
> +
> + memory_error:
> + virReportOOMError();
> + virNetlinkCallbackDataFree(calld);
> +
> + return -1;
> +}
> +
>
> /**
> * virNetDevMacVLanCreateWithVPortProfile:
> @@ -810,7 +853,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
> int retries, do_retry = 0;
> uint32_t macvtapMode;
> const char *cr_ifname;
> - virNetlinkCallbackDataPtr calld = NULL;
> int ret;
> int vf = -1;
>
> @@ -917,36 +959,12 @@ create_name:
> goto disassociate_exit;
> }
>
> - if (virtPortProfile && virNetlinkEventServiceIsRunning()) {
> - if (VIR_ALLOC(calld) < 0)
> - goto memory_error;
> - if ((calld->cr_ifname = strdup(cr_ifname)) == NULL)
> - goto memory_error;
> - if (VIR_ALLOC(calld->virtPortProfile) < 0)
> - goto memory_error;
> - memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile));
> - if (VIR_ALLOC_N(calld->macaddress, VIR_MAC_BUFLEN) < 0)
> - goto memory_error;
> - memcpy(calld->macaddress, macaddress, VIR_MAC_BUFLEN);
> - if ((calld->linkdev = strdup(linkdev)) == NULL)
> - goto memory_error;
> - if (VIR_ALLOC_N(calld->vmuuid, VIR_UUID_BUFLEN) < 0)
> - goto memory_error;
> - memcpy(calld->vmuuid, vmuuid, VIR_UUID_BUFLEN);
> -
> - calld->vmOp = vmOp;
> -
> - virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
> - virNetDevMacVLanVPortProfileDestroyCallback,
> - calld, macaddress);
> - }
> + if (virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress,
> + linkdev, vmuuid, virtPortProfile, vmOp) < 0 )
> + goto disassociate_exit;
>
> return rc;
>
> - memory_error:
> - virReportOOMError();
> - virNetlinkCallbackDataFree(calld);
> -
> disassociate_exit:
> ignore_value(virNetDevVPortProfileDisassociate(cr_ifname,
> virtPortProfile,
> @@ -1003,6 +1021,48 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
> return ret;
> }
>
> +/**
> + * virNetDevMacVLanRestartWithVPortProfile:
> + * Register a port profile callback handler for a VM that
> + * is already running
> + * .
> + * @cr_ifname: Interface name that the macvtap has.
> + * @macaddress: The MAC address for the macvtap device
> + * @linkdev: The interface name of the NIC to connect to the external bridge
> + * @vmuuid: The UUID of the VM the macvtap belongs to
> + * @virtPortProfile: pointer to object holding the virtual port profile data
> + * @vmOp: Operation to use during setup of the association
> + *
> + * Returns 0; returns -1 on error.
> + */
> +int virNetDevMacVLanRestartWithVPortProfile(const char *cr_ifname,
> + const unsigned char *macaddress,
> + const char *linkdev,
> + const unsigned char *vmuuid,
> + virNetDevVPortProfilePtr virtPortProfile,
> + enum virNetDevVPortProfileOp vmOp)
> +{
> + int rc = 0;
> +
> + rc = virNetDevMacVLanVPortProfileRegisterCallback(cr_ifname, macaddress,
> + linkdev, vmuuid,
> + virtPortProfile, vmOp);
> + if (rc < 0)
> + goto error;
> +
> + ignore_value(virNetDevVPortProfileAssociate(cr_ifname,
> + virtPortProfile,
> + macaddress,
> + linkdev,
> + -1,
> + vmuuid,
> + vmOp, true));
Related to this patch - while looking at the uses of
virNetDevVPortProfileAssociate(), I found the function
qemuMigrationVPAssociatePortProfiles() in src/qemu/qemu_migration.c. It
calls virNetDevVPortProfileAssociate() without ever calling
virNetDevMacVLanVPortProfileRegisterCallback(). I'm wondering if either
1) the normal setup of port profiles (is
virNetDevMacVLanCreateWithVPortProfile) is missed during migration, and
this function needs to be changed/enhanced, or 2) this associate is
redundant. I don't have the hardware to test either a migration *or*
vepa mode, but this seems like something important to determine (and fix
if it's broken).
> +
> +error:
> + return rc;
> +
> +}
> +
> #else /* ! WITH_MACVTAP */
> int virNetDevMacVLanCreate(const char *ifname ATTRIBUTE_UNUSED,
> const char *type ATTRIBUTE_UNUSED,
> @@ -1052,4 +1112,16 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname ATTRIBUTE_UNUSED,
> _("Cannot create macvlan devices on this platform"));
> return -1;
> }
> +
> +int virNetDevMacVLanRestartWithVPortProfile(const char *cr_ifname ATTRIBUTE_UNUSED,
> + const unsigned char *macaddress ATTRIBUTE_UNUSED,
> + const char *linkdev ATTRIBUTE_UNUSED,
> + const unsigned char *vmuuid ATTRIBUTE_UNUSED,
> + virNetDevVPortProfilePtr virtPortProfile ATTRIBUTE_UNUSED,
> + enum virNetDevVPortProfileOp vmOp ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s",
> + _("Cannot create macvlan devices on this platform"));
> + return -1;
> +}
> #endif /* ! WITH_MACVTAP */
> diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
> index 130ecea..14640cf 100644
> --- a/src/util/virnetdevmacvlan.h
> +++ b/src/util/virnetdevmacvlan.h
> @@ -75,4 +75,13 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> ATTRIBUTE_NONNULL(6) ATTRIBUTE_RETURN_CHECK;
>
> +int virNetDevMacVLanRestartWithVPortProfile(const char *cr_ifname,
> + const unsigned char *macaddress,
> + const char *linkdev,
> + const unsigned char *vmuuid,
> + virNetDevVPortProfilePtr virtPortProfile,
> + enum virNetDevVPortProfileOp vmOp)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
> + ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK;
> +
> #endif /* __UTIL_MACVTAP_H__ */
I've made all the changes I noted above, and included them in the
patchfile I've attached to this message. I don't want to push them
untested, though, and don't have the hardware to test. Can you squash my
patch into your patch and test it? If it works, resend your patch + my
patch as a single patch, and I'll push that.
Thanks!
>From 4585452f0b012393266275d1f31d20e940560a46 Mon Sep 17 00:00:00 2001
From: Laine Stump <laine@xxxxxxxxx>
Date: Fri, 23 Mar 2012 14:18:57 -0400
Subject: [PATCH] nits to squash into "qemu,util: on restart of libvirt
restart vepa callbacks"
---
src/qemu/qemu_driver.c | 13 +++++++------
src/util/virnetdevmacvlan.c | 11 ++++++-----
src/util/virnetlink.c | 6 +++++-
3 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b1f45dc..4524a99 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -418,7 +418,7 @@ cleanup:
static void qemuDomainNetsRestart(void *payload,
const void *name ATTRIBUTE_UNUSED,
- void *data)
+ void *data ATTRIBUTE_UNUSED)
{
int i;
virDomainObjPtr vm = (virDomainObjPtr)payload;
@@ -428,14 +428,15 @@ static void qemuDomainNetsRestart(void *payload,
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
- if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT &&
- net->data.direct.mode == VIR_NETDEV_MACVLAN_MODE_VEPA) {
- VIR_DEBUG("Device active on %s in VEPA mode. Reassociating.",net->ifname);
+ if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT &&
+ virDomainNetGetActualDirectMode(net) == VIR_NETDEV_MACVLAN_MODE_VEPA) {
+ VIR_DEBUG("VEPA mode device %s active in domain %s. Reassociating.",
+ net->ifname, def->name);
ignore_value(virNetDevMacVLanRestartWithVPortProfile(net->ifname,
net->mac,
- net->data.direct.linkdev,
+ virDomainNetGetActualDirectDev(net),
def->uuid,
- net->data.direct.virtPortProfile,
+ virDomainNetGetActualVirtPortProfile(net),
VIR_NETDEV_VPORT_PROFILE_OP_CREATE));
}
}
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 156de3c..37891b4 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -783,17 +783,18 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
calld->vmOp = vmOp;
- virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
- virNetDevMacVLanVPortProfileDestroyCallback,
- calld, macaddress);
+ if (virNetlinkEventAddClient(virNetDevMacVLanVPortProfileCallback,
+ virNetDevMacVLanVPortProfileDestroyCallback,
+ calld, macaddress) < 0)
+ goto error;
}
return 0;
- memory_error:
+memory_error:
virReportOOMError();
+error:
virNetlinkCallbackDataFree(calld);
-
return -1;
}
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index feb0fc7..ed2c545 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -428,8 +428,11 @@ virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB,
int i, r, ret = -1;
virNetlinkEventSrvPrivatePtr srv = server;
- if (handleCB == NULL)
+ if (handleCB == NULL) {
+ netlinkError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Invalid NULL callback provided"));
return -1;
+ }
virNetlinkEventServerLock(srv);
@@ -449,6 +452,7 @@ virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB,
srv->handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT);
if (VIR_RESIZE_N(srv->handles, srv->handlesAlloc,
srv->handlesCount, NETLINK_EVENT_ALLOC_EXTENT) < 0) {
+ virReportOOMError();
goto error;
}
}
--
1.7.7.6
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list