On 2012年09月14日 21:32, Peter Krempa wrote:
On 09/13/12 08:56, Osier Yang wrote:
This improve helper vshStringToArray to accept const string as
s/improve/improves/
argument instead. To not convert the const string when using
vshStringToArray, and thus avoid motifying it.
I'd write the last sentence as:
This avoids modifying const strings when using vshStringToArray.
v4 - v5:
* Except both are PATCH 6/7, totally different patches.
Remove this before pushing please.
---
tools/virsh-domain.c | 2 +-
tools/virsh-pool.c | 2 +-
tools/virsh.c | 10 ++++++----
tools/virsh.h | 2 +-
4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index c6695b3..18422b7 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2337,7 +2337,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
vshUndefineVolume *vlist = NULL;
int nvols = 0;
const char *volumes_arg = NULL;
- char *volumes = NULL;
+ const char *volumes = NULL;
The volumes variable will need to be removed completely with the new
functionality. In cmdUndefine the volumes string was the copy of the
const argument that you're trying to avoid.
Hum, right, I didn't check it carefully.
The unfortunate thing here is that "volumes" is used very wildly across
cmdUndefine. But the cure should be easy: you need to rename volumes_arg
to volumes. But this poses another problem:
Your function now allocates two things, but the previous callers (that
you didn't update free only one)
The first one is the array of strings that is populated with the string
fragments. This variable is freed normaly. The second one is the string
copy that is exploded into the array. And you don't free this one.
Fortunately, the way strsep and your code works has one advantage you
might use: The first element in the array is always pointing to the
beginning of the tokenized string (in your case to the memory you
allocated for the copy). So to free this you might want to do something
like:
Oh, right.
VIR_FREE(*token_var);
VIR_FREE(token_var);
For this I'd go with a macro that does this (and adds a check as
token_var might be NULL).
hmm. I don't have a good idea for the macro name though.
char **volume_tokens = NULL;
char *volume_tok = NULL;
int nvolume_tokens = 0;
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index 15d1883..2ca7a18 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -856,7 +856,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd
ATTRIBUTE_UNUSED)
char **poolTypes = NULL;
int npoolTypes = 0;
- npoolTypes = vshStringToArray((char *)type, &poolTypes);
+ npoolTypes = vshStringToArray(type, &poolTypes);
You'll need to call the cleanup macro or anything you implement for
freeing poolTypes.
Okay.
for (i = 0; i < npoolTypes; i++) {
if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) {
diff --git a/tools/virsh.c b/tools/virsh.c
index 242f789..d0b302a 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -174,19 +174,20 @@ vshPrettyCapacity(unsigned long long val, const
char **unit)
* on error.
*/
int
-vshStringToArray(char *str,
+vshStringToArray(const char *str,
char ***array)
{
+ char *str_copied = vshStrdup(NULL, str);
char *str_tok = NULL;
unsigned int nstr_tokens = 0;
char **arr = NULL;
/* tokenize the string from user and save it's parts into an array */
- if (str) {
+ if (str_copied) {
nstr_tokens = 1;
/* count the delimiters */
- str_tok = str;
+ str_tok = str_copied;
while (*str_tok) {
if (*str_tok == ',')
nstr_tokens++;
@@ -195,12 +196,13 @@ vshStringToArray(char *str,
if (VIR_ALLOC_N(arr, nstr_tokens) < 0) {
virReportOOMError();
+ VIR_FREE(str_copied);
return -1;
}
/* tokenize the input string */
nstr_tokens = 0;
- str_tok = str;
+ str_tok = str_copied;
do {
arr[nstr_tokens] = strsep(&str_tok, ",");
nstr_tokens++;
diff --git a/tools/virsh.h b/tools/virsh.h
index 30eff4b..1220079 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -330,7 +330,7 @@ int vshAskReedit(vshControl *ctl, const char *msg);
int vshStreamSink(virStreamPtr st, const char *bytes, size_t nbytes,
void *opaque);
double vshPrettyCapacity(unsigned long long val, const char **unit);
-int vshStringToArray(char *str, char ***array);
+int vshStringToArray(const char *str, char ***array);
/* Typedefs, function prototypes for job progress reporting.
* There are used by some long lingering commands like
A nice improvement. The rest of the code looks okay, but I'd like to see
the fixed version before you push.
Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
