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(a)redhat.com +1-919-301-3266
Libvirt virtualization library
http://libvirt.org