[libvirt] [PATCH 1/4] parallels: Resolve issues with uninitialized 'ret' value

[Daniel P. Berrange]

On Tue, Jan 08, 2013 at 10:40:58AM -0500, John Ferlan wrote:
> Added some messaging to indicate possible failure from virXPathULongLong()
> as well
> ---
>  src/parallels/parallels_storage.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c
> index 2908bee..10206bf 100644
> --- a/src/parallels/parallels_storage.c
> +++ b/src/parallels/parallels_storage.c
> @@ -258,7 +258,7 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml,
>                                        virStorageVolDefPtr def)
>  {
>      xmlXPathContextPtr ctxt = NULL;
> -    int ret;
> +    int ret = -1;
>  
>      if (STRNEQ((const char *)root->name, "Parallels_disk_image")) {
>          virReportError(VIR_ERR_XML_ERROR,
> @@ -274,12 +274,17 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml,
>  
>      ctxt->node = root;
>  
> -    if (virXPathULongLong("string(./Disk_Parameters/Disk_size)",
> -                          ctxt, &def->capacity))
> -        ret = -1;
> +    if ((ret = virXPathULongLong("string(./Disk_Parameters/Disk_size)",
> +                                 ctxt, &def->capacity)) < 0) {

No need to assign to 'ret' here. It is initialized to -1 right at the
top, and set to 0 on success later. So this intermediate assingment
is a latent bug waiting to happen.

> +        virReportError(VIR_ERR_XML_ERROR,
> +                       "%s", _("failed to get disk size from "
> +                               "the disk descriptor xml"));
> +        goto cleanup;
> +    }
>  
>      def->capacity <<= 9;
>      def->allocation = def->capacity;
> +    ret = 0;
>  cleanup:
>      xmlXPathFreeContext(ctxt);
>      return ret;
> @@ -315,7 +320,8 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool,
>  
>      def->type = VIR_STORAGE_VOL_FILE;
>  
> -    parallelsDiskDescParse(diskDescPath, def);
> +    if (parallelsDiskDescParse(diskDescPath, def) < 0)
> +        goto no_parse;
>  
>      if (!(def->target.path = realpath(diskPath, NULL)))
>          goto no_memory;
> @@ -333,6 +339,9 @@ no_memory:
>      virStorageVolDefFree(def);
>      virReportOOMError();
>      return -1;
> +no_parse:
>  }

Generally we aim to only have 3 label names 'error' or 'cleanup' or
'no_memory'. Inthis case you want 'error'. I'd refactor it to allow
fallthrough from 'no_memory' to 'error' thus:

   no_memory:
    virReportOOMError();
   error:
    virStorageVolDefFree(def);
    return -1;

which is our normal coding pattern for this scenario.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|