Re: [libvirt] PATCH: 6/28: Reduce return points in QEMU driver
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/