Re: [libvirt] [PATCH 6/7 v5] virsh: Don't motify the const string

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.

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:

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).

> 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.

> > 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