Re: [libvirt] [PATCH] qemu: Avoid holding the driver lock in trivial snapshot API's
On 09/24/12 18:51, Eric Blake wrote:
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...
Ok, I'll get rid of this.
> +/* 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).
Yes, there's a lot of API's that don't need the driver locked. I just
chose a subset so this patch doesn't grow out of control and is hard to
review. I also wanted to establish a pattern with this that we can use
later on, as the usual way to add new API's is to copy something existing.
>
> 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.
Posting v2 now.