[libvirt] [PATCH 3/3] virsh: Don't leak list of volumes when undefining domain with storage
Osier Yang
jyang at redhat.com
Tue Aug 20 09:58:29 UTC 2013
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
More information about the libvir-list
mailing list