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

[libvirt] PATCH: Misc fixes to qparams



I found a couple more small bugs in the qparams code

 - In the qparam_query_parse() method, after appending each (name,value)
   pair of params, it failed to free the temporary buffers for the 
   (name,value) pair.
 - Did not allow for ';' as a valid query parameter separator
 - In a couple of OOM cleanup scenarios it failed to free buffers allocated
   earlier on

In looking at this I decide we ought to have a test suite for this code
so I'm also including one. It has 100% coverage of all the non-OOM code
paths. The test case now passes when run under valgrind


[berrange t60wlan libvirt-numa]$ diffstat .hg/patches/qparam-test 
 b/tests/qparamtest.c       |  224 +++++++++++++++++++++++++++++++++++++++++++++
 scripts/coverage-report.pl |    3 
 src/qparams.c              |   26 ++++-
 tests/Makefile.am          |    8 +
 4 files changed, 253 insertions(+), 8 deletions(-)


Dan.

diff -r aa244ae10e9e scripts/coverage-report.pl
--- a/scripts/coverage-report.pl	Wed May 21 19:13:06 2008 -0400
+++ b/scripts/coverage-report.pl	Wed May 21 19:42:55 2008 -0400
@@ -65,6 +65,9 @@
     } elsif (/^Lines executed:(.*)%\s*of\s*(\d+)\s*$/) {
 	$coverage{$type}->{$name}->{lines} = $2;
 	$coverage{$type}->{$name}->{linesCoverage} = $1;
+    } elsif (/^No executable lines\s*$/) {
+	$coverage{$type}->{$name}->{lines} = 0;
+	$coverage{$type}->{$name}->{linesCoverage} = "100.00";
     } elsif (/^Branches executed:(.*)%\s*of\s*(\d+)\s*$/) {
 	$coverage{$type}->{$name}->{branches} = $2;
 	$coverage{$type}->{$name}->{branchesCoverage} = $1;
diff -r aa244ae10e9e src/qparams.c
--- a/src/qparams.c	Wed May 21 19:13:06 2008 -0400
+++ b/src/qparams.c	Wed May 21 19:42:55 2008 -0400
@@ -166,7 +166,7 @@
 qparam_query_parse (const char *query)
 {
     struct qparam_set *ps;
-    const char *name, *value, *end, *eq;
+    const char *end, *eq;
 
     ps = new_qparam_set (0, NULL);
     if (!ps) return NULL;
@@ -174,9 +174,14 @@
     if (!query || query[0] == '\0') return ps;
 
     while (*query) {
+        char *name = NULL, *value = NULL;
+
         /* Find the next separator, or end of the string. */
         end = strchr (query, '&');
-        if (!end) end = query + strlen (query);
+        if (!end)
+            end = strchr (query, ';');
+        if (!end)
+            end = query + strlen (query);
 
         /* Find the first '=' character between here and end. */
         eq = strchr (query, '=');
@@ -191,7 +196,6 @@
          */
         else if (!eq) {
             name = xmlURIUnescapeString (query, end - query, NULL);
-            value = "";
             if (!name) goto out_of_memory;
         }
         /* Or if we have "name=" here (works around annoying
@@ -199,7 +203,6 @@
          */
         else if (eq+1 == end) {
             name = xmlURIUnescapeString (query, eq - query, NULL);
-            value = "";
             if (!name) goto out_of_memory;
         }
         /* If the '=' character is at the beginning then we have
@@ -211,12 +214,23 @@
         /* Otherwise it's "name=value". */
         else {
             name = xmlURIUnescapeString (query, eq - query, NULL);
+            if (!name)
+                goto out_of_memory;
             value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL);
-            if (!name || !value) goto out_of_memory;
+            if (!value) {
+                VIR_FREE(name);
+                goto out_of_memory;
+            }
         }
 
         /* Append to the parameter set. */
-        if (append_qparam (ps, name, value) == -1) goto out_of_memory;
+        if (append_qparam (ps, name, value ? value : "") == -1) {
+            VIR_FREE(name);
+            VIR_FREE(value);
+            goto out_of_memory;
+        }
+        VIR_FREE(name);
+        VIR_FREE(value);
 
     next:
         query = end;
diff -r aa244ae10e9e tests/Makefile.am
--- a/tests/Makefile.am	Wed May 21 19:13:06 2008 -0400
+++ b/tests/Makefile.am	Wed May 21 19:42:55 2008 -0400
@@ -41,7 +41,7 @@
 
 noinst_PROGRAMS = xmlrpctest xml2sexprtest sexpr2xmltest virshtest conftest \
 	reconnect xmconfigtest xencapstest qemuxml2argvtest qemuxml2xmltest \
-        nodeinfotest statstest
+        nodeinfotest statstest qparamtest
 
 test_scripts = \
 	daemon-conf \
@@ -53,7 +53,7 @@
 
 TESTS = xml2sexprtest sexpr2xmltest virshtest test_conf.sh xmconfigtest \
         xencapstest qemuxml2argvtest qemuxml2xmltest nodeinfotest \
-	statstest $(test_scripts)
+	statstest qparamtest $(test_scripts)
 if ENABLE_XEN_TESTS
   TESTS += reconnect
 endif
@@ -130,6 +130,10 @@
 	statstest.c testutils.h testutils.c
 statstest_LDADD = $(LDADDS)
 
+qparamtest_SOURCES = \
+	qparamtest.c testutils.h testutils.c
+qparamtest_LDADD = $(LDADDS)
+
 reconnect_SOURCES = \
 	reconnect.c
 reconnect_LDADD = $(LDADDS)
diff -r aa244ae10e9e tests/qparamtest.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tests/qparamtest.c	Wed May 21 19:42:55 2008 -0400
@@ -0,0 +1,224 @@
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "testutils.h"
+#include "qparams.h"
+#include "util.h"
+
+struct qparamParseDataEntry {
+    const char *name;
+    const char *value;
+};
+
+struct qparamParseData {
+    const char *queryIn;
+    const char *queryOut;
+    int nparams;
+    const struct qparamParseDataEntry *params;
+};
+
+static int
+qparamParseTest(const void *data)
+{
+    const struct qparamParseData *expect = data;
+    struct qparam_set *actual = qparam_query_parse(expect->queryIn);
+    int ret = -1, i;
+    if (!actual)
+        return -1;
+
+    if (actual->n != expect->nparams)
+        goto fail;
+
+    for (i = 0 ; i < actual->n ; i++) {
+        if (!STREQ(expect->params[i].name,
+                   actual->p[i].name))
+            goto fail;
+        if (!STREQ(expect->params[i].value,
+                   actual->p[i].value))
+            goto fail;
+    }
+
+    ret = 0;
+
+fail:
+    free_qparam_set(actual);
+    return ret;
+}
+
+static int
+qparamFormatTest(const void *data)
+{
+    const struct qparamParseData *expect = data;
+    struct qparam_set *actual = qparam_query_parse(expect->queryIn);
+    char *output = NULL;
+    int ret = -1;
+
+    if (!actual)
+        return -1;
+
+    output = qparam_get_query(actual);
+    if (!output)
+        goto fail;
+
+    if (!STREQ(output, expect->queryOut))
+        goto fail;
+
+    ret = 0;
+
+fail:
+    free(output);
+    free_qparam_set(actual);
+    return ret;
+}
+
+static int
+qparamBuildTest(const void *data)
+{
+    const struct qparamParseData *expect = data;
+    struct qparam_set *actual = new_qparam_set(0, NULL);
+    int ret = -1, i;
+    if (!actual)
+        return -1;
+
+    for (i = 0 ; i < expect->nparams ; i++) {
+        if (append_qparam(actual,
+                          expect->params[i].name,
+                          expect->params[i].value) < 0)
+            goto fail;
+    }
+
+    if (actual->n != expect->nparams)
+        goto fail;
+
+    for (i = 0 ; i < actual->n ; i++) {
+        if (!STREQ(expect->params[i].name,
+                   actual->p[i].name))
+            goto fail;
+        if (!STREQ(expect->params[i].value,
+                   actual->p[i].value))
+            goto fail;
+    }
+
+    ret = 0;
+
+fail:
+    free_qparam_set(actual);
+    return ret;
+}
+
+
+static int
+qparamTestNewVargs(const void *data ATTRIBUTE_UNUSED)
+{
+    struct qparam_set *actual = new_qparam_set(0, "foo", "one", "bar", "two", NULL);
+    int ret = -1;
+    if (!actual)
+        return -1;
+
+    if (actual->n != 2)
+        goto fail;
+
+    if (!STREQ(actual->p[0].name, "foo"))
+        goto fail;
+
+    if (!STREQ(actual->p[0].value, "one"))
+        goto fail;
+
+    if (!STREQ(actual->p[1].name, "bar"))
+        goto fail;
+
+    if (!STREQ(actual->p[1].value, "two"))
+        goto fail;
+
+    ret = 0;
+
+fail:
+    free_qparam_set(actual);
+    return ret;
+}
+
+static int
+qparamTestAddVargs(const void *data ATTRIBUTE_UNUSED)
+{
+    struct qparam_set *actual = new_qparam_set(0, NULL);
+    int ret = -1;
+    if (!actual)
+        return -1;
+
+    if (append_qparams(actual,  "foo", "one", "bar", "two", NULL) < 0)
+        goto fail;
+
+    if (actual->n != 2)
+        goto fail;
+
+    if (!STREQ(actual->p[0].name, "foo"))
+        goto fail;
+
+    if (!STREQ(actual->p[0].value, "one"))
+        goto fail;
+
+    if (!STREQ(actual->p[1].name, "bar"))
+        goto fail;
+
+    if (!STREQ(actual->p[1].value, "two"))
+        goto fail;
+
+    ret = 0;
+
+fail:
+    free_qparam_set(actual);
+    return ret;
+}
+
+static const struct qparamParseDataEntry params1[] = { { "foo", "one" }, { "bar", "two" } };
+static const struct qparamParseDataEntry params2[] = { { "foo", "one" }, { "foo", "two" } };
+static const struct qparamParseDataEntry params3[] = { { "foo", "&one" }, { "bar", "&two" } };
+static const struct qparamParseDataEntry params4[] = { { "foo", "" } };
+static const struct qparamParseDataEntry params5[] = { { "foo", "one two" } };
+static const struct qparamParseDataEntry params6[] = { { "foo", "one" } };
+
+int
+main(void)
+{
+    int ret = 0;
+
+#define DO_TEST(queryIn,queryOut,params)                                \
+    do {                                                                \
+        struct qparamParseData info = {                                 \
+            queryIn,                                                    \
+            queryOut ? queryOut : queryIn,                              \
+            sizeof(params)/sizeof(params[0]),                           \
+            params };                                                   \
+        if (virtTestRun("Parse " queryIn,                               \
+                        1, qparamParseTest, &info) < 0)                 \
+            ret = -1;                                                   \
+        if (virtTestRun("Format " queryIn,                              \
+                        1, qparamFormatTest, &info) < 0)                \
+            ret = -1;                                                   \
+        if (virtTestRun("Build " queryIn,                               \
+                        1, qparamBuildTest, &info) < 0)                 \
+            ret = -1;                                                   \
+    } while (0)
+
+
+    DO_TEST("foo=one&bar=two", NULL, params1);
+    DO_TEST("foo=one&foo=two", NULL, params2);
+    DO_TEST("foo=one&&foo=two", "foo=one&foo=two", params2);
+    DO_TEST("foo=one;foo=two", "foo=one&foo=two", params2);
+    DO_TEST("foo", "foo=", params4);
+    DO_TEST("foo=", NULL, params4);
+    DO_TEST("foo=&", "foo=", params4);
+    DO_TEST("foo=&&", "foo=", params4);
+    DO_TEST("foo=one%20two", NULL, params5);
+    DO_TEST("=bogus&foo=one", "foo=one", params6);
+
+    if (virtTestRun("New vargs", 1, qparamTestNewVargs, NULL) < 0)
+        ret = -1;
+    if (virtTestRun("Add vargs", 1, qparamTestAddVargs, NULL) < 0)
+        ret = -1;
+
+    exit(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
+}


-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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