On 20/08/13 17:05, Peter Krempa wrote:
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
---
tools/virsh-domain.c | 121 ++++++++++++++++++++++++---------------------------
1 file changed, 58 insertions(+), 63 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 13e3045..a4b81a7 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2932,12 +2932,11 @@ 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;
+ vshUndefineVolume *vols = NULL;
+ size_t nvols = 0;
+ const char *vol_list = NULL;
+ char **volumes = NULL;
+ int nvolumes = 0;
Better, but still confused, e.g. "vols" and "volumes". How about:
vshUndefineVolume *vol_list = NULL;
const char *vol_string = NULL;
char **vol_array = NULL;
hope it's not even worse. :-)
char *def = NULL;
char *source = NULL;
char *target = NULL;
@@ -2946,10 +2945,9 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
xmlDocPtr doc = NULL;
xmlXPathContextPtr ctxt = NULL;
xmlNodePtr *vol_nodes = NULL;
- int nvolumes = 0;
- bool vol_not_found = false;
+ int nvol_nodes;
Hm, "vol_*" is better more now.
- ignore_value(vshCommandOptString(cmd, "storage", &volumes));
+ ignore_value(vshCommandOptString(cmd, "storage", &vol_list));
if (managed_save) {
flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE;
@@ -3017,14 +3015,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
}
/* Stash domain description for later use */
- if (volumes || remove_all_storage) {
+ if (vol_list || 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"));
I think we will want to change it into "inactive domains", but it's
another story.
goto cleanup;
}
- if (volumes && remove_all_storage) {
- vshError(ctl, _("Specified both --storage and --remove-all-storage"));
+ if (vol_list && remove_all_storage) {
+ vshError(ctl,
+ _("Specified both --storage and --remove-all-storage"));
goto cleanup;
}
@@ -3038,20 +3039,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_list &&
+ (nvolumes = vshStringToArray(vol_list, &volumes)) < 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 +3061,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;
Is is fine to remove this checking?
- 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;
+ bool found = false;
+ for (j = 0; j < nvolumes; j++) {
+ if (STREQ_NULLABLE(volumes[j], target) ||
+ STREQ_NULLABLE(volumes[j], source)) {
+ VIR_FREE(volumes[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;
+ bool found = false;
+ for (i = 0; i < nvolumes; i++) {
+ if (volumes[i]) {
+ vshError(ctl,
+ _("Volume '%s' was not found in domain's "
+ "definition.\n"), volumes[i]);
+ found = true;
}
}
- if (vol_not_found)
+ if (found)
goto cleanup;
}
}
@@ -3173,9 +3168,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 +3180,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 +3195,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);
- if (volume_tokens) {
- VIR_FREE(*volume_tokens);
- VIR_FREE(volume_tokens);
- }
+ for (i = 0; i < nvolumes; i++)
+ VIR_FREE(volumes[i]);
You can use virStringListFree now.
Others look good.
Osier
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list