On Sun, Nov 30, 2008 at 11:31:56PM +0000, Daniel P. Berrange wrote:
This patch reduces the number of return points in the QEMU driver methods centralizing all cleanup code. It also removes a bunch of unnecessary type casts, since void * automatically casts to any type. Finally it separates out variable declarations from variable initializations since the initializations will need to be protected with the locked critical section.
IMHO the superfluous cast were harmless so could have been preserved, but it's not a big deal, [...]
+ if (vm->state != VIR_DOMAIN_PAUSED) { + if (qemudMonitorCommand(driver, vm, "stop", &info) < 0) { + qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, + "%s", _("suspend operation failed")); + goto cleanup; + } + vm->state = VIR_DOMAIN_PAUSED; + qemudDebug("Reply %s", info); + qemudDomainEventDispatch(driver, vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); + VIR_FREE(info);
qemudMonitorCommand has no comment on when the reply field may or not be initialized. The code shows it's only if it returns 0 but that's worth a comment or VIR_FREE(info); should be moved in the cleanup part. The code is right but any small change here may generate a leak, and that's true for most of the entry points. [...]
static int qemudDomainSave(virDomainPtr dom, const char *path) {
+cleanup: + if (fd != -1) + close(fd); + VIR_FREE(xml); + VIR_FREE(safe_path); + VIR_FREE(command); + VIR_FREE(info); + if (ret != 0) + unlink(path); + + return ret; }
Considering the complexity for the function, I would not be surprized if that patch isn't cleaning up a couple of leaks on errors. Good to have a systematic freeing. [...]
@@ -2271,15 +2346,15 @@ static int qemudDomainRestore(virConnect def))) { qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("failed to assign new VM")); - virDomainDefFree(def); - close(fd); - return -1; + goto cleanup; } + def = NULL;
Hum, okay, but that function is harder to review [...]
static int qemudDomainStart(virDomainPtr dom) { [...] ret = qemudStartVMDaemon(dom->conn, driver, vm, NULL); - if (ret < 0) - return ret; - qemudDomainEventDispatch(driver, vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_BOOTED); - return 0; + if (ret != -1) + qemudDomainEventDispatch(driver, vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_BOOTED);
small semantic change, but since -1 is the only error code, fine [...]
/* Return the disks name for use in monitor commands */ -static char *qemudDiskDeviceName(const virDomainPtr dom, +static char *qemudDiskDeviceName(const virConnectPtr conn, const virDomainDiskDefPtr disk) {
a bit fo refactoring [...]
-static int qemudDomainChangeEjectableMedia(virDomainPtr dom, +static int qemudDomainChangeEjectableMedia(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, virDomainDeviceDefPtr dev)
here too [...]
- VIR_FREE(dev); +cleanup: + virDomainDeviceDefFree(dev); return ret; }
hum, that was a leak here apparently [...]
+ if (asprintf(&cmd, "pci_del 0 %d", detach->slotnum) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, NULL); + cmd = NULL;
If memory allocation wasn’t possible, or some other error occurs, these functions will return -1, and the contents of strp is undefined. gasp, fine :-) Okay, independantly of the threading this sis a good cleanup patch which should be applied, +1 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/