[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]