[libvirt] [PATCH v2 1/1] qemu, util: on restart of libvirt restart vepa callbacks

Laine Stump laine at laine.org
Fri Mar 23 18:21:47 UTC 2012


On 03/21/2012 09:21 AM, D. Herrendoerfer wrote:
> From: "D. Herrendoerfer" <d.herrendoerfer at herrendoerfer.name>
>
> 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 at herrendoerfer.name>
> ---
>  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!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-nits-to-squash-into-qemu-util-on-restart-of-libvirt-.patch
Type: text/x-patch
Size: 4133 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120323/1f5a33f3/attachment-0001.bin>


More information about the libvir-list mailing list