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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list