[libvirt] [PATCH] qemu: Avoid holding the driver lock in trivial snapshot API's

[Eric Blake]

On 09/24/2012 06:57 AM, Peter Krempa wrote:
> In most of the snapshot API's there's no need to hold the driver lock
> the whole time.
> 
> This patch adds helper functions that get the domain object in functions
> that don't require the driver lock and simplifies call paths from
> snapshot-related API's.
> ---
>  src/conf/domain_conf.c |   3 +-
>  src/qemu/qemu_driver.c | 306 +++++++++++++++----------------------------------
>  2 files changed, 94 insertions(+), 215 deletions(-)
> 

>  void virDomainObjUnlock(virDomainObjPtr obj)
>  {
> -    virMutexUnlock(&obj->lock);
> +    if (obj)
> +        virMutexUnlock(&obj->lock);
>  }

I agree with Daniel that this hunk is bad.  But most of the patch is
worthwhile...

> +/* Looks up the domain obj and unlocks the driver */
> +static virDomainObjPtr
> +qemuDomObjFromDomain(virDomainPtr domain)
> +{
> +    struct qemud_driver *driver = domain->conn->privateData;
> +    virDomainObjPtr vm;
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> +    qemuDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, domain->uuid);
> +    qemuDriverUnlock(driver);
> +    if (!vm) {
> +        virUUIDFormat(domain->uuid, uuidstr);
> +        virReportError(VIR_ERR_NO_DOMAIN,
> +                       _("no domain with matching uuid '%s'"), uuidstr);
> +    }
> +
> +    return vm;
> +}

This is a nice function for one-shot lookups, when the rest of the
function does not need the driver locked.


> @@ -11305,59 +11359,39 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names,
>                                         int nameslen,
>                                         unsigned int flags)
>  {
> -    struct qemud_driver *driver = domain->conn->privateData;
>      virDomainObjPtr vm = NULL;
>      int n = -1;
> 
>      virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS |
>                    VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1);
> 
> -    qemuDriverLock(driver);
> -    vm = virDomainFindByUUID(&driver->domains, domain->uuid);
> -    if (!vm) {
> -        char uuidstr[VIR_UUID_STRING_BUFLEN];
> -        virUUIDFormat(domain->uuid, uuidstr);
> -        virReportError(VIR_ERR_NO_DOMAIN,
> -                       _("no domain with matching uuid '%s'"), uuidstr);
> +    if (!(vm = qemuDomObjFromDomain(domain)))
>          goto cleanup;
> -    }

And this is a nice usage of the new helpers (probably a LOT more APIs
that can use them, rather than just snapshots).

> 
>      n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen,
>                                           flags);
> 
>  cleanup:
> -    if (vm)
> -        virDomainObjUnlock(vm);
> -    qemuDriverUnlock(driver);
> +    virDomainObjUnlock(vm);
>      return n;

This hunk, however, should be:

cleanup:
    if (vm)
        virDomainObjUnlock(vm);
    return n;

Looking forward to v2.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120924/6e1b85d2/attachment-0001.sig>