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

On Tue, Nov 6, 2012 at 10:00 PM, Eric Blake <eblake(a)redhat.com&gt; 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().

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

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

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