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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list