5:13 a.m.
On 08/20/13 11:58, Osier Yang wrote:
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. :-)
Yeah, we need 3 sets of variables and it's getting confusing ...
> 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.
Yeah, seems so.
>
> - 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.
Okay, may be worth doing when respinning.
> 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;
> }
>
...
> @@ -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?
Yes it is. The check was bogous as it was actually expecting that the
error from virXPathString would raise the error and store it in
last_error which will not happen as it's not an RPC call and would
require calling vshSaveLibvirtError.
The code later on can't undefine volumes without a source anyways. The
only thing this was meant to catch is an XML parsing problem, but this
was so unlikely that we can chose to ignore it.
> - 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]);
Please note this line ....
> + found = true;
> break;
> }
> }
> - if (!volume_tok)
> + if (!found)
> continue;
> }
>
// trimmed a few lines as formatting was broken anyways
> @@ -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.
You can't: The code above is freeing individual items if they are found
later on, thus this array needs to be freed manually and every item
needs to be checked. virStringListFree is working on NULL terminated arrays.
Others look good.
Osier
Peter