At a slightly larger memory expense allow stealing of items from the string array returned from vshStringToArray and turn the result into a string list compatible with virStringSplit. This will allow to use the common dealloc function. This patch also fixes a few forgotten checks of return from vshStringToArray and one memory leak. --- Notes: Version 2: * tweaked comment of vshStringToArray to mention deallocation of the returned array tools/virsh-nodedev.c | 18 +++++------------- tools/virsh-pool.c | 10 ++++------ tools/virsh-snapshot.c | 10 ++-------- tools/virsh.c | 15 ++++++++------- tools/virsh.h | 1 + 5 files changed, 20 insertions(+), 34 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 3255079..cfbf3bc 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -161,10 +161,7 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - if (arr) { - VIR_FREE(*arr); - VIR_FREE(arr); - } + virStringFreeList(arr); virNodeDeviceFree(dev); return ret; } @@ -409,7 +406,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) vshError(ctl, "%s", _("Options --tree and --cap are incompatible")); return false; } - ncaps = vshStringToArray(cap_str, &caps); + if ((ncaps = vshStringToArray(cap_str, &caps)) < 0) + return false; } for (i = 0; i < ncaps; i++) { @@ -503,10 +501,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } cleanup: - if (caps) { - VIR_FREE(*caps); - VIR_FREE(caps); - } + virStringFreeList(caps); vshNodeDeviceListFree(list); return ret; } @@ -574,10 +569,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: - if (arr) { - VIR_FREE(*arr); - VIR_FREE(arr); - } + virStringFreeList(arr); VIR_FREE(xml); virNodeDeviceFree(device); return ret; diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 1622be2..b8fc8d7 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -995,12 +995,13 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) char **poolTypes = NULL; int npoolTypes = 0; - npoolTypes = vshStringToArray(type, &poolTypes); + if ((npoolTypes = vshStringToArray(type, &poolTypes)) < 0) + return false; for (i = 0; i < npoolTypes; i++) { if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) { vshError(ctl, "%s", _("Invalid pool type")); - VIR_FREE(poolTypes); + virStringFreeList(poolTypes); return false; } @@ -1036,10 +1037,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) break; } } - if (poolTypes) { - VIR_FREE(*poolTypes); - VIR_FREE(poolTypes); - } + virStringFreeList(poolTypes); } if (!(list = vshStoragePoolListCollect(ctl, flags))) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index db9715b..e37a5b3 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -261,10 +261,7 @@ vshParseSnapshotMemspec(vshControl *ctl, virBufferPtr buf, const char *str) cleanup: if (ret < 0) vshError(ctl, _("unable to parse memspec: %s"), str); - if (array) { - VIR_FREE(*array); - VIR_FREE(array); - } + virStringFreeList(array); return ret; } @@ -313,10 +310,7 @@ vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str) cleanup: if (ret < 0) vshError(ctl, _("unable to parse diskspec: %s"), str); - if (array) { - VIR_FREE(*array); - VIR_FREE(array); - } + virStringFreeList(array); return ret; } diff --git a/tools/virsh.c b/tools/virsh.c index f65dc79..15f529e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -163,10 +163,9 @@ vshPrettyCapacity(unsigned long long val, const char **unit) } /* - * Convert the strings separated by ',' into array. The caller - * must free the first array element and the returned array after - * use (all other array elements belong to the memory allocated - * for the first array element). + * Convert the strings separated by ',' into array. The returned + * array is a NULL terminated string list. The caller has to free + * the array using virStringFreeList or a similar method. * * Returns the length of the filled array on success, or -1 * on error. @@ -196,7 +195,8 @@ vshStringToArray(const char *str, str_tok++; } - if (VIR_ALLOC_N(arr, nstr_tokens) < 0) { + /* reserve the NULL element at the end */ + if (VIR_ALLOC_N(arr, nstr_tokens + 1) < 0) { VIR_FREE(str_copied); return -1; } @@ -212,12 +212,13 @@ vshStringToArray(const char *str, continue; } *tmp++ = '\0'; - arr[nstr_tokens++] = str_tok; + arr[nstr_tokens++] = vshStrdup(NULL, str_tok); str_tok = tmp; } - arr[nstr_tokens++] = str_tok; + arr[nstr_tokens++] = vshStrdup(NULL, str_tok); *array = arr; + VIR_FREE(str_copied); return nstr_tokens; } diff --git a/tools/virsh.h b/tools/virsh.h index a407428..466ca2d 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -37,6 +37,7 @@ # include "virerror.h" # include "virthread.h" # include "virnetdevbandwidth.h" +# include "virstring.h" # define VSH_MAX_XML_FILE (10*1024*1024) -- 1.8.3.2

Use the new semantics of vshStringToArray to avoid leaking the array of volumes to be deleted. The array would be leaked in case the first volume was found in the domain definition. Also refactor the code a bit to sanitize naming of variables hoding arrays and dimensions of the arrays. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=996050 --- Notes: Version 2: * renamed a few variables: - vol_list -> vol_string - volumes -> vol_list and the corresponding count variables. * reordered the declaration order ov variables tools/virsh-domain.c | 134 +++++++++++++++++++++++++-------------------------- 1 file changed, 65 insertions(+), 69 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 13e3045..b29f934 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2932,24 +2932,23 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) int rc = -1; int running; /* list of volumes to remove along with this domain */ - vshUndefineVolume *vlist = NULL; - int nvols = 0; - const char *volumes = NULL; - char **volume_tokens = NULL; - char *volume_tok = NULL; - int nvolume_tokens = 0; - char *def = NULL; + const char *vol_string = NULL; /* string containing volumes to delete */ + char **vol_list = NULL; /* tokenized vol_string */ + int nvol_list = 0; + vshUndefineVolume *vols = NULL; /* info about the volumes to delete*/ + size_t nvols = 0; + char *def = NULL; /* domain def */ + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlNodePtr *vol_nodes = NULL; /* XML nodes of volumes of the guest */ + int nvol_nodes; char *source = NULL; char *target = NULL; size_t i; size_t j; - xmlDocPtr doc = NULL; - xmlXPathContextPtr ctxt = NULL; - xmlNodePtr *vol_nodes = NULL; - int nvolumes = 0; - bool vol_not_found = false; - ignore_value(vshCommandOptString(cmd, "storage", &volumes)); + + ignore_value(vshCommandOptString(cmd, "storage", &vol_string)); if (managed_save) { flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; @@ -3017,14 +3016,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) } /* Stash domain description for later use */ - if (volumes || remove_all_storage) { + if (vol_string || remove_all_storage) { if (running) { - vshError(ctl, _("Storage volume deletion is supported only on stopped domains")); + vshError(ctl, + _("Storage volume deletion is supported only on " + "stopped domains")); goto cleanup; } - if (volumes && remove_all_storage) { - vshError(ctl, _("Specified both --storage and --remove-all-storage")); + if (vol_string && remove_all_storage) { + vshError(ctl, + _("Specified both --storage and --remove-all-storage")); goto cleanup; } @@ -3038,20 +3040,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) goto error; /* tokenize the string from user and save it's parts into an array */ - if (volumes) { - if ((nvolume_tokens = vshStringToArray(volumes, &volume_tokens)) < 0) - goto cleanup; - } - - if ((nvolumes = virXPathNodeSet("./devices/disk", ctxt, - &vol_nodes)) < 0) + if (vol_string && + (nvol_list = vshStringToArray(vol_string, &vol_list)) < 0) goto error; - if (nvolumes > 0) - vlist = vshCalloc(ctl, nvolumes, sizeof(*vlist)); + if ((nvol_nodes = virXPathNodeSet("./devices/disk", ctxt, + &vol_nodes)) < 0) + goto error; - for (i = 0; i < nvolumes; i++) { + for (i = 0; i < nvol_nodes; i++) { ctxt->node = vol_nodes[i]; + vshUndefineVolume vol; VIR_FREE(source); VIR_FREE(target); @@ -3063,56 +3062,53 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) "./source/@file|" "./source/@dir|" "./source/@name|" - "./source/@dev)", ctxt))) { - if (last_error && last_error->code != VIR_ERR_OK) - goto error; - else - continue; - } + "./source/@dev)", ctxt))) + continue; /* lookup if volume was selected by user */ - if (volumes) { - volume_tok = NULL; - for (j = 0; j < nvolume_tokens; j++) { - if (volume_tokens[j] && - (STREQ(volume_tokens[j], target) || - STREQ(volume_tokens[j], source))) { - volume_tok = volume_tokens[j]; - volume_tokens[j] = NULL; + if (vol_list) { + bool found = false; + for (j = 0; j < nvol_list; j++) { + if (STREQ_NULLABLE(vol_list[j], target) || + STREQ_NULLABLE(vol_list[j], source)) { + VIR_FREE(vol_list[j]); + found = true; break; } } - if (!volume_tok) + if (!found) continue; } - if (!(vlist[nvols].vol = virStorageVolLookupByPath(ctl->conn, - source))) { + if (!(vol.vol = virStorageVolLookupByPath(ctl->conn, source))) { vshPrint(ctl, _("Storage volume '%s'(%s) is not managed by libvirt. " "Remove it manually.\n"), target, source); vshResetLibvirtError(); continue; } - vlist[nvols].source = source; - vlist[nvols].target = target; + + vol.source = source; + vol.target = target; source = NULL; target = NULL; - nvols++; + if (VIR_APPEND_ELEMENT(vols, nvols, vol) < 0) + goto cleanup; } /* print volumes specified by user that were not found in domain definition */ - if (volumes) { - for (j = 0; j < nvolume_tokens; j++) { - if (volume_tokens[j]) { - vshError(ctl, _("Volume '%s' was not found in domain's " - "definition.\n"), - volume_tokens[j]); - vol_not_found = true; + if (vol_list) { + bool found = false; + for (i = 0; i < nvol_list; i++) { + if (vol_list[i]) { + vshError(ctl, + _("Volume '%s' was not found in domain's " + "definition.\n"), vol_list[i]); + found = true; } } - if (vol_not_found) + if (found) goto cleanup; } } @@ -3173,9 +3169,9 @@ out: for (i = 0; i < nvols; i++) { if (wipe_storage) { vshPrint(ctl, _("Wiping volume '%s'(%s) ... "), - vlist[i].target, vlist[i].source); + vols[i].target, vols[i].source); fflush(stdout); - if (virStorageVolWipe(vlist[i].vol, 0) < 0) { + if (virStorageVolWipe(vols[i].vol, 0) < 0) { vshError(ctl, _("Failed! Volume not removed.")); ret = false; continue; @@ -3185,13 +3181,13 @@ out: } /* delete the volume */ - if (virStorageVolDelete(vlist[i].vol, 0) < 0) { + if (virStorageVolDelete(vols[i].vol, 0) < 0) { vshError(ctl, _("Failed to remove storage volume '%s'(%s)"), - vlist[i].target, vlist[i].source); + vols[i].target, vols[i].source); ret = false; } else { vshPrint(ctl, _("Volume '%s'(%s) removed.\n"), - vlist[i].target, vlist[i].source); + vols[i].target, vols[i].source); } } } @@ -3200,17 +3196,17 @@ cleanup: VIR_FREE(source); VIR_FREE(target); for (i = 0; i < nvols; i++) { - VIR_FREE(vlist[i].source); - VIR_FREE(vlist[i].target); - if (vlist[i].vol) - virStorageVolFree(vlist[i].vol); + VIR_FREE(vols[i].source); + VIR_FREE(vols[i].target); + if (vols[i].vol) + virStorageVolFree(vols[i].vol); } - VIR_FREE(vlist); + VIR_FREE(vols); + + for (i = 0; i < nvol_list; i++) + VIR_FREE(vol_list[i]); + VIR_FREE(vol_list); - if (volume_tokens) { - VIR_FREE(*volume_tokens); - VIR_FREE(volume_tokens); - } VIR_FREE(def); VIR_FREE(vol_nodes); xmlFreeDoc(doc); -- 1.8.3.2