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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list