[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 :|