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

[Osier Yang]

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