[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 11/07/12 06:41, Doug Goldstein wrote:
On Tue, Nov 6, 2012 at 10:00 PM, Eric Blake <eblake 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]