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

[Peter Krempa]

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