4:05 a.m.
Usage of vshStringToArray in virsh was problematic in a few places.
Clean it up to avoid a few memleaks. Also improve an error message.
Peter Krempa (3):
virsh: modify vshStringToArray to duplicate the elements too
virsh-pool: Improve error message in cmdPoolList
virsh: Don't leak list of volumes when undefining domain with storage
tools/virsh-domain.c | 121 ++++++++++++++++++++++++-------------------------
tools/virsh-nodedev.c | 18 ++------
tools/virsh-pool.c | 12 ++---
tools/virsh-snapshot.c | 10 +---
tools/virsh.c | 8 ++--
tools/virsh.h | 1 +
6 files changed, 76 insertions(+), 94 deletions(-)
--
1.8.3.2
4:05 a.m.
New subject: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too
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.
---
tools/virsh-nodedev.c | 18 +++++-------------
tools/virsh-pool.c | 10 ++++------
tools/virsh-snapshot.c | 10 ++--------
tools/virsh.c | 8 +++++---
tools/virsh.h | 1 +
5 files changed, 17 insertions(+), 30 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..d6e3cb3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -196,7 +196,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 +213,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
4:16 a.m.
New subject: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too
On 20/08/13 17:05, Peter Krempa wrote:
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.
---
tools/virsh-nodedev.c | 18 +++++-------------
tools/virsh-pool.c | 10 ++++------
tools/virsh-snapshot.c | 10 ++--------
tools/virsh.c | 8 +++++---
tools/virsh.h | 1 +
5 files changed, 17 insertions(+), 30 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..d6e3cb3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -196,7 +196,8 @@ vshStringToArray(const char *str,
Why not to destroy the use of vshStringToArray, and convert to
virStringSplit? and the comment of vshStringToArray should be updated,
since the method for free'ing the memory is different now, if we keep
using it.
Osier
4:20 a.m.
New subject: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too
On 08/20/13 11:16, Osier Yang wrote:
On 20/08/13 17:05, Peter Krempa wrote:
> 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.
> ---
> tools/virsh-nodedev.c | 18 +++++-------------
> tools/virsh-pool.c | 10 ++++------
> tools/virsh-snapshot.c | 10 ++--------
> tools/virsh.c | 8 +++++---
> tools/virsh.h | 1 +
> 5 files changed, 17 insertions(+), 30 deletions(-)
>
...
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index f65dc79..d6e3cb3 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -196,7 +196,8 @@ vshStringToArray(const char *str,
Why not to destroy the use of vshStringToArray, and convert to
vshStringToArray is converting double commas (",,") as escape sequence
for a comma. virStringSplit doesn't do this. We would regress in virsh
option parsing if we'd convert it to virStringSplit.
virStringSplit? and the comment of vshStringToArray should be
updated,
since the method for free'ing the memory is different now, if we keep
using it.
Okay, I'll touch up the comment. I didn't notice it contained info how
to free it.
Peter
5:01 a.m.
New subject: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too
On 20/08/13 17:20, Peter Krempa wrote:
On 08/20/13 11:16, Osier Yang wrote:
> On 20/08/13 17:05, Peter Krempa wrote:
>> 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.
>> ---
>> tools/virsh-nodedev.c | 18 +++++-------------
>> tools/virsh-pool.c | 10 ++++------
>> tools/virsh-snapshot.c | 10 ++--------
>> tools/virsh.c | 8 +++++---
>> tools/virsh.h | 1 +
>> 5 files changed, 17 insertions(+), 30 deletions(-)
>>
...
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index f65dc79..d6e3cb3 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -196,7 +196,8 @@ vshStringToArray(const char *str,
> Why not to destroy the use of vshStringToArray, and convert to
vshStringToArray is converting double commas (",,") as escape sequence
for a comma. virStringSplit doesn't do this. We would regress in virsh
option parsing if we'd convert it to virStringSplit.
Integrating the escaping into virStringSplit with a flag will work,
but I'm not against if you keep using it, since the mainly purpose
of these patches is to fix the bug.
> virStringSplit? and the comment of vshStringToArray should be updated,
> since the method for free'ing the memory is different now, if we keep
> using it.
Okay, I'll touch up the comment. I didn't notice it contained info how
to free it.
Peter
5:25 a.m.
New subject: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too
On 08/20/13 12:01, Osier Yang wrote:
On 20/08/13 17:20, Peter Krempa wrote:
> On 08/20/13 11:16, Osier Yang wrote:
>> On 20/08/13 17:05, Peter Krempa wrote:
>>> 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.
>>> ---
>>> tools/virsh-nodedev.c | 18 +++++-------------
>>> tools/virsh-pool.c | 10 ++++------
>>> tools/virsh-snapshot.c | 10 ++--------
>>> tools/virsh.c | 8 +++++---
>>> tools/virsh.h | 1 +
>>> 5 files changed, 17 insertions(+), 30 deletions(-)
>>>
> ...
>
>>> diff --git a/tools/virsh.c b/tools/virsh.c
>>> index f65dc79..d6e3cb3 100644
>>> --- a/tools/virsh.c
>>> +++ b/tools/virsh.c
>>> @@ -196,7 +196,8 @@ vshStringToArray(const char *str,
>> Why not to destroy the use of vshStringToArray, and convert to
> vshStringToArray is converting double commas (",,") as escape sequence
> for a comma. virStringSplit doesn't do this. We would regress in virsh
> option parsing if we'd convert it to virStringSplit.
Integrating the escaping into virStringSplit with a flag will work,
but I'm not against if you keep using it, since the mainly purpose
of these patches is to fix the bug.
It won't be that easy for virStringSplit. virStringSplit is splitting
the string on a delimiter _string_ thus it won't be easy to teach it
escape sequences. The double comma escape is a virsh thing, thus I
didn't bother changing that globally (and risk potential regressions in
code that is using virStringSplit).
Peter
4:05 a.m.
New subject: [libvirt] [PATCH 2/3] virsh-pool: Improve error message in cmdPoolList
Explicitly let the user know about the unknown volume name.
---
tools/virsh-pool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index b8fc8d7..592b81f 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -1000,7 +1000,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
for (i = 0; i < npoolTypes; i++) {
if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) {
- vshError(ctl, "%s", _("Invalid pool type"));
+ vshError(ctl, _("Invalid pool type '%s'"),
poolTypes[i]);
virStringFreeList(poolTypes);
return false;
}
--
1.8.3.2
4:05 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: Don't leak list of volumes when undefining domain with storage
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;
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;
- 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"));
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;
- 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]);
+
+ VIR_FREE(volumes);
VIR_FREE(def);
VIR_FREE(vol_nodes);
xmlFreeDoc(doc);
--
1.8.3.2
4:58 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: Don't leak list of volumes when undefining domain with storage
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
5:13 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: Don't leak list of volumes when undefining domain with storage
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
4112
days inactive
4112
days old
10 comments
2 participants
participants (2)
-
Osier Yang -
Peter Krempa