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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list