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 :|