[libvirt] [PATCH 1/2] virsh: make ,, escape parsing common

[Peter Krempa]

On 11/07/12 06:41, Doug Goldstein wrote:
> On Tue, Nov 6, 2012 at 10:00 PM, Eric Blake <eblake at redhat.com> wrote:
>> So far, none of the existing callers of vshStringToArray expected
>> the user to ever pass a literal comma; meanwhile, snapshot parsing
>> had rolled its own array parser.  Moving the comma escaping into
>> the common function won't affect any existing callers, and will make
>> this function reusable for adding memory handling to snapshot parsing.
>>
>> As a bonus, the testsuite was already testing snapshot parsing, so
>> the fact that the test still passes means that we are now giving
>> testsuite exposure to vshStringToArray.
>>
>> * tools/virsh-snapshot.c (vshParseSnapshotDiskspec): Move ,,
>> parsing...
>> * tools/virsh.c (vshStringToArray): ...into common function.
>> Also, vshStrdup can't fail.
>> ---
>>   tools/virsh-snapshot.c | 50 ++++++++++++++++++++++-------------------------
>>   tools/virsh.c          | 53 ++++++++++++++++++++++++++++++--------------------
>>   2 files changed, 55 insertions(+), 48 deletions(-)
>>
>> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
>> index 818ef56..159f577 100644
>> --- a/tools/virsh-snapshot.c
>> +++ b/tools/virsh-snapshot.c
>> @@ -197,33 +197,26 @@ static int
>>   vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
>>   {
>>       int ret = -1;
>> -    char *name = NULL;
>> -    char *snapshot = NULL;
>> -    char *driver = NULL;
>> -    char *file = NULL;
>> -    char *spec = vshStrdup(ctl, str);
>> -    char *tmp = spec;
>> -    size_t len = strlen(str);
>> -
>> -    if (*str == ',')
>> +    const char *name = NULL;
>> +    const char *snapshot = NULL;
>> +    const char *driver = NULL;
>> +    const char *file = NULL;
>> +    char **array = NULL;
>> +    int narray;
>> +    int i;
>> +
>> +    narray = vshStringToArray(str, &array);
>> +    if (narray <= 0)
>>           goto cleanup;
>> -    name = tmp;
>> -    while ((tmp = strchr(tmp, ','))) {
>> -        if (tmp[1] == ',') {
>> -            /* Recognize ,, as an escape for a literal comma */
>> -            memmove(&tmp[1], &tmp[2], len - (tmp - spec) - 2 + 1);
>> -            len--;
>> -            tmp++;
>> -            continue;
>> -        }
>> -        /* Terminate previous string, look for next recognized one */
>> -        *tmp++ = '\0';
>> -        if (!snapshot && STRPREFIX(tmp, "snapshot="))
>> -            snapshot = tmp + strlen("snapshot=");
>> -        else if (!driver && STRPREFIX(tmp, "driver="))
>> -            driver = tmp + strlen("driver=");
>> -        else if (!file && STRPREFIX(tmp, "file="))
>> -            file = tmp + strlen("file=");
>> +
>> +    name = array[0];
>> +    for (i = 1; i < narray; i++) {
>> +        if (!snapshot && STRPREFIX(array[i], "snapshot="))
>> +            snapshot = array[i] + strlen("snapshot=");
>> +        else if (!driver && STRPREFIX(array[i], "driver="))
>> +            driver = array[i] + strlen("driver=");
>> +        else if (!file && STRPREFIX(array[i], "file="))
>> +            file = array[i] + strlen("file=");
>>           else
>>               goto cleanup;
>>       }
>> @@ -245,7 +238,10 @@ vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
>>   cleanup:
>>       if (ret < 0)
>>           vshError(ctl, _("unable to parse diskspec: %s"), str);
>> -    VIR_FREE(spec);
>> +    if (array) {
>> +        VIR_FREE(*array);
>> +        VIR_FREE(array);
>> +    }
>>       return ret;
>>   }
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index a5585e1..9598b75 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -179,35 +179,46 @@ vshStringToArray(const char *str,
>>   {
>>       char *str_copied = vshStrdup(NULL, str);
>>       char *str_tok = NULL;
>> +    char *tmp;
>>       unsigned int nstr_tokens = 0;
>>       char **arr = NULL;
>> +    size_t len = strlen(str_copied);
>
> If str_copied is NULL due to an OOM (I think that's what vshStrdup
> does on OOM) or if someone was bad with their call to
> vshStringToArray() we'll go boom on this strlen().

vshStrdup calls exit() if it hits OOM so this part is OK.

>
>>
>>       /* tokenize the string from user and save it's parts into an array */
>> -    if (str_copied) {

As is taking this out of this condition.

>> -        nstr_tokens = 1;
>> -
>> -        /* count the delimiters */
>> -        str_tok = str_copied;
>> -        while (*str_tok) {
>> -            if (*str_tok == ',')
>> -                nstr_tokens++;
>> +    nstr_tokens = 1;
>> +
>> +    /* count the delimiters, recognizing ,, as an escape for a
>> +     * literal comma */
>> +    str_tok = str_copied;
>> +    while ((str_tok = strchr(str_tok, ','))) {
>> +        if (str_tok[1] == ',')
>>               str_tok++;
>> -        }
>> +        else
>> +            nstr_tokens++;
>> +        str_tok++;
>> +    }
>>
>> -        if (VIR_ALLOC_N(arr, nstr_tokens) < 0) {
>> -            virReportOOMError();
>> -            VIR_FREE(str_copied);
>> -            return -1;
>> -        }
>> +    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_copied;
>> -        do {
>> -            arr[nstr_tokens] = strsep(&str_tok, ",");
>> -            nstr_tokens++;
>> -        } while (str_tok);
>> +    /* tokenize the input string */
>> +    nstr_tokens = 0;
>> +    tmp = str_tok = str_copied;
>> +    while ((tmp = strchr(tmp, ','))) {
>> +        if (tmp[1] == ',') {
>> +            memmove(&tmp[1], &tmp[2], len - (tmp - str_copied) - 2 + 1);
>> +            len--;
>> +            tmp++;
>
> Not a review but just trying to understand it (maybe a short comment
> above for others like me that needed to pause to look at this would be
> nice) but basically you're just shifting off one of the double commas
> right? The code above looks correct for that.

I agree a line explaining what that part does would be nice, but not 
necessary.

>
>> +            continue;
>> +        }
>> +        *tmp++ = '\0';
>> +        arr[nstr_tokens++] = str_tok;
>> +        str_tok = tmp;
>>       }
>> +    arr[nstr_tokens++] = str_tok;
>>
>>       *array = arr;
>>       return nstr_tokens;
>> --
>> 1.7.11.7
>
> Just 1 little nit for the strlen(), which its possible we guarantee we
> won't hit that. And then a clarification. But otherwise this looks
> good so visual ACK. I haven't pulled it down and compiled it or syntax
> checked it.
>

ACK with or without the comment added.

Peter