[libvirt] [PATCH 0/4] qemu: Refactor ManagedSave code and fix error reporting
This series improves helpers to retrieve domain objects from virDomainPtrs and uses the improvements in the ManagedSave APIs. This series also fixes a bug in error reporting of qemuDomainManagedSaveRemove where if unlink() failed no error was reported but an error code was returned. Although there's a slight net increase of number of lines by this patch, it's mainly due to the docs added in 1/4 Peter Krempa (4): qemu: Add a new domain lookup helper and improve the docs qemu: Refactor managed save functions to use domain lookup helpers qemu: Small code cleanups in the managedsave functions qemu: Improve error reporting from qemuDomainManagedSaveRemove src/qemu/qemu_driver.c | 115 +++++++++++++++++++++++++++---------------------- 1 file changed, 63 insertions(+), 52 deletions(-) -- 1.8.0
This patch adds a new domain lookup helper qemuDomObjFromDomainDriver that lookups the domain and leaves the driver locked. The driver is returned as the second argument of that function. If the lookup fails the driver is unlocked to help avoid cleanup codepaths. This patch also improves docs for the helpers. --- src/qemu/qemu_driver.c | 45 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 93d9b69..1f43386 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -187,11 +187,20 @@ struct qemuAutostartData { virConnectPtr conn; }; - -/* Looks up the domain object and unlocks the driver. The returned domain - * object is locked and the caller is responsible for unlocking it. */ +/** + * qemuDomObjFromDomainDriver: + * @domain: Domain pointer that has to be looked up + * @drv: Pointer to virQEMUDriverPtr to return the driver object + * + * This fucntion looks up @domain in the domain list and returns the + * appropriate virDomainObjPtr. On sucessful lookup, both driver and domain + * object are returned locked. + * + * Returns the domain object if it's found and the driver. Both are locked. + * In case of failure NULL is returned and the driver isn't locked. + */ static virDomainObjPtr -qemuDomObjFromDomain(virDomainPtr domain) +qemuDomObjFromDomainDriver(virDomainPtr domain, virQEMUDriverPtr *drv) { virQEMUDriverPtr driver = domain->conn->privateData; virDomainObjPtr vm; @@ -199,16 +208,42 @@ qemuDomObjFromDomain(virDomainPtr domain) 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); + qemuDriverUnlock(driver); + *drv = NULL; + return NULL; } + *drv = driver; return vm; } +/** + * qemuDomObjFromDomain: + * @domain: Domain pointer that has to be looked up + * + * This function looks up @domain and returns the appropriate + * virDomainObjPtr. The driver is unlocked after the call. + * + * Returns the domain object which is locked on success, NULL + * otherwise. Te driver remains unlocked after the call. + */ +static virDomainObjPtr +qemuDomObjFromDomain(virDomainPtr domain) +{ + virDomainObjPtr vm; + virQEMUDriverPtr driver; + + if (!(vm = qemuDomObjFromDomainDriver(domain, &driver))) + return NULL; + + qemuDriverUnlock(driver); + + return vm; +} /* Looks up the domain object from snapshot and unlocks the driver. The * returned domain object is locked and the caller is responsible for -- 1.8.0
On 12/11/2012 11:48 AM, Peter Krempa wrote:
This patch adds a new domain lookup helper qemuDomObjFromDomainDriver that lookups the domain and leaves the driver locked. The driver is returned as the second argument of that function. If the lookup fails the driver is unlocked to help avoid cleanup codepaths.
This patch also improves docs for the helpers. --- src/qemu/qemu_driver.c | 45 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 5 deletions(-)
+/** + * qemuDomObjFromDomainDriver: + * @domain: Domain pointer that has to be looked up + * @drv: Pointer to virQEMUDriverPtr to return the driver object + * + * This fucntion looks up @domain in the domain list and returns the
s/fucntion/function/
+ * appropriate virDomainObjPtr. On sucessful lookup, both driver and domain
s/sucessful/successful/
+ * object are returned locked. + * + * Returns the domain object if it's found and the driver. Both are locked. + * In case of failure NULL is returned and the driver isn't locked. + */
+/** + * qemuDomObjFromDomain: + * @domain: Domain pointer that has to be looked up + * + * This function looks up @domain and returns the appropriate + * virDomainObjPtr. The driver is unlocked after the call. + * + * Returns the domain object which is locked on success, NULL + * otherwise. Te driver remains unlocked after the call.
s/Te/The/ ACK with spelling fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/qemu/qemu_driver.c | 54 +++++++++++--------------------------------------- 1 file changed, 12 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1f43386..72da014 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3246,8 +3246,8 @@ qemuDomainManagedSavePath(virQEMUDriverPtr driver, virDomainObjPtr vm) { static int qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; - virDomainObjPtr vm = NULL; + virQEMUDriverPtr driver; + virDomainObjPtr vm; char *name = NULL; int ret = -1; int compressed; @@ -3256,15 +3256,8 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED, -1); - qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = qemuDomObjFromDomainDriver(dom, &driver))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -3323,50 +3316,28 @@ cleanup: static int qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - int ret = -1; virCheckFlags(0, -1); - qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } - - ret = vm->hasManagedSave; + if (!(vm = qemuDomObjFromDomain(dom))) + return -1; -cleanup: - if (vm) - virDomainObjUnlock(vm); - qemuDriverUnlock(driver); - return ret; + return vm->hasManagedSave; } static int qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; - virDomainObjPtr vm = NULL; + virQEMUDriverPtr driver; + virDomainObjPtr vm; int ret = -1; char *name = NULL; virCheckFlags(0, -1); - qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = qemuDomObjFromDomainDriver(dom, &driver))) + return -1; name = qemuDomainManagedSavePath(driver, vm); if (name == NULL) @@ -3377,8 +3348,7 @@ qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) cleanup: VIR_FREE(name); - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } -- 1.8.0
On 12/11/2012 11:48 AM, Peter Krempa wrote:
--- src/qemu/qemu_driver.c | 54 +++++++++++--------------------------------------- 1 file changed, 12 insertions(+), 42 deletions(-)
Nice reduction in size. However, you need a v2...
@@ -3323,50 +3316,28 @@ cleanup: static int qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - int ret = -1;
virCheckFlags(0, -1);
- qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } - - ret = vm->hasManagedSave; + if (!(vm = qemuDomObjFromDomain(dom))) + return -1;
-cleanup: - if (vm) - virDomainObjUnlock(vm); - qemuDriverUnlock(driver); - return ret; + return vm->hasManagedSave; }
Ouch. This leaves vm locked on exit. It needs to look more like: if !(vm = qemuDomObjFromDomain(dom))) return -1; ret = vm->hasManagedSave; virDomainObjUnlock(vm); retun ret; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/11/12 22:50, Eric Blake wrote:
On 12/11/2012 11:48 AM, Peter Krempa wrote:
--- src/qemu/qemu_driver.c | 54 +++++++++++--------------------------------------- 1 file changed, 12 insertions(+), 42 deletions(-)
Nice reduction in size. However, you need a v2...
[...]
-cleanup: - if (vm) - virDomainObjUnlock(vm); - qemuDriverUnlock(driver); - return ret; + return vm->hasManagedSave; }
Ouch. This leaves vm locked on exit. It needs to look more like:
Removal of code is really addictive :) I got carried away and strived for more lines :) V2 comming in soon.
if !(vm = qemuDomObjFromDomain(dom))) return -1; ret = vm->hasManagedSave; virDomainObjUnlock(vm); retun ret;
Peter
--- Diff to v1: -unlock the domain in qemuDomainHasManagedSaveImage() --- src/qemu/qemu_driver.c | 53 +++++++++++++------------------------------------- 1 file changed, 13 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9173413..4144e6f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3238,8 +3238,8 @@ qemuDomainManagedSavePath(virQEMUDriverPtr driver, virDomainObjPtr vm) { static int qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; - virDomainObjPtr vm = NULL; + virQEMUDriverPtr driver; + virDomainObjPtr vm; char *name = NULL; int ret = -1; int compressed; @@ -3248,15 +3248,8 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED, -1); - qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = qemuDomObjFromDomainDriver(dom, &driver))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -3315,50 +3308,31 @@ cleanup: static int qemuDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - int ret = -1; + int ret; virCheckFlags(0, -1); - qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = qemuDomObjFromDomain(dom))) + return -1; ret = vm->hasManagedSave; - -cleanup: - if (vm) - virDomainObjUnlock(vm); - qemuDriverUnlock(driver); + virDomainObjUnlock(vm); return ret; } static int qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; - virDomainObjPtr vm = NULL; + virQEMUDriverPtr driver; + virDomainObjPtr vm; int ret = -1; char *name = NULL; virCheckFlags(0, -1); - qemuDriverLock(driver); - vm = virDomainFindByUUID(&driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = qemuDomObjFromDomainDriver(dom, &driver))) + return -1; name = qemuDomainManagedSavePath(driver, vm); if (name == NULL) @@ -3369,8 +3343,7 @@ qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) cleanup: VIR_FREE(name); - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } -- 1.8.0
On 12/12/2012 12:17 AM, Peter Krempa wrote:
--- Diff to v1: -unlock the domain in qemuDomainHasManagedSaveImage() ---
src/qemu/qemu_driver.c | 53 +++++++++++++------------------------------------- 1 file changed, 13 insertions(+), 40 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/12/12 14:23, Eric Blake wrote:
On 12/12/2012 12:17 AM, Peter Krempa wrote:
--- Diff to v1: -unlock the domain in qemuDomainHasManagedSaveImage() ---
src/qemu/qemu_driver.c | 53 +++++++++++++------------------------------------- 1 file changed, 13 insertions(+), 40 deletions(-)
ACK.
Thanks for the review. I pushed the series. Peter
Save a few lines moving assignments into conditions and fix braces position. --- src/qemu/qemu_driver.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 72da014..b1cb185 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3232,7 +3232,8 @@ qemuDomainSave(virDomainPtr dom, const char *path) } static char * -qemuDomainManagedSavePath(virQEMUDriverPtr driver, virDomainObjPtr vm) { +qemuDomainManagedSavePath(virQEMUDriverPtr driver, virDomainObjPtr vm) +{ char *ret; if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name) < 0) { @@ -3270,8 +3271,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) goto cleanup; } - name = qemuDomainManagedSavePath(driver, vm); - if (name == NULL) + if (!(name = qemuDomainManagedSavePath(driver, vm))) goto cleanup; VIR_INFO("Saving state to %s", name); @@ -3339,8 +3339,7 @@ qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) if (!(vm = qemuDomObjFromDomainDriver(dom, &driver))) return -1; - name = qemuDomainManagedSavePath(driver, vm); - if (name == NULL) + if (!(name = qemuDomainManagedSavePath(driver, vm))) goto cleanup; ret = unlink(name); -- 1.8.0
On 12/11/2012 11:48 AM, Peter Krempa wrote:
Save a few lines moving assignments into conditions and fix braces position. --- src/qemu/qemu_driver.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
ACK. Cosmetic, but brings it more in line with our style. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Report an error if unlink of the managedsave file fails. --- Hm, this patch changes semantics a little bit. If the unlink() fails now, the domain isn't marked as not having a managed save file. If somebody does not like this change I will return it to the previous semantics. --- src/qemu/qemu_driver.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b1cb185..1941ae7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3342,8 +3342,15 @@ qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) if (!(name = qemuDomainManagedSavePath(driver, vm))) goto cleanup; - ret = unlink(name); + if (unlink(name) < 0) { + virReportSystemError(errno, + _("Failed to remove managed save file '%s'"), + name); + goto cleanup; + } + vm->hasManagedSave = false; + ret = 0; cleanup: VIR_FREE(name); -- 1.8.0
On 12/11/2012 11:48 AM, Peter Krempa wrote:
Report an error if unlink of the managedsave file fails. --- Hm, this patch changes semantics a little bit. If the unlink() fails now, the domain isn't marked as not having a managed save file. If somebody does not like this change I will return it to the previous semantics.
The new semantics make sense - another libvirtd restart will see the managedsave file still existing, and can attempt to reload/re-unlink it again. ACK as-is.
--- src/qemu/qemu_driver.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b1cb185..1941ae7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3342,8 +3342,15 @@ qemuDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) if (!(name = qemuDomainManagedSavePath(driver, vm))) goto cleanup;
- ret = unlink(name); + if (unlink(name) < 0) { + virReportSystemError(errno, + _("Failed to remove managed save file '%s'"), + name); + goto cleanup; + } + vm->hasManagedSave = false; + ret = 0;
cleanup: VIR_FREE(name);
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa