[libvirt] [PATCH 0/3] Fix gluster volume lookup of multiple volumes
Fix the broken XPath and add tests. Peter Krempa (3): storage: util: Split out the gluster volume extraction code into new function test: Introduce testing of virStorageUtilGlusterExtractPoolSources storage: Fix XPath for looking up gluster volume name src/storage/storage_util.c | 93 +++++++++++-------- src/storage/storage_util.h | 4 + tests/Makefile.am | 15 ++- .../virstorageutildata/gluster-parse-basic-dst.xml | 6 ++ .../virstorageutildata/gluster-parse-basic-src.xml | 47 ++++++++++ .../gluster-parse-multivol-dst.xml | 14 +++ .../gluster-parse-multivol-src.xml | 32 +++++++ tests/virstorageutiltest.c | 103 +++++++++++++++++++++ 8 files changed, 272 insertions(+), 42 deletions(-) create mode 100644 tests/virstorageutildata/gluster-parse-basic-dst.xml create mode 100644 tests/virstorageutildata/gluster-parse-basic-src.xml create mode 100644 tests/virstorageutildata/gluster-parse-multivol-dst.xml create mode 100644 tests/virstorageutildata/gluster-parse-multivol-src.xml create mode 100644 tests/virstorageutiltest.c -- 2.12.1
To allow testing of the algorithm, split out the extractor into a separate helper. --- src/storage/storage_util.c | 93 +++++++++++++++++++++++++++------------------- src/storage/storage_util.h | 4 ++ 2 files changed, 58 insertions(+), 39 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 7687eb89a..8459e9d5b 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2836,6 +2836,59 @@ virStorageBackendDeleteLocal(virConnectPtr conn ATTRIBUTE_UNUSED, } +int +virStorageUtilGlusterExtractPoolSources(const char *host, + const char *xml, + int pooltype, + virStoragePoolSourceListPtr list) +{ + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlNodePtr *nodes = NULL; + virStoragePoolSource *src = NULL; + size_t i; + int nnodes; + int ret = -1; + + if (!(doc = virXMLParseStringCtxt(xml, _("(gluster_cli_output)"), &ctxt))) + goto cleanup; + + if ((nnodes = virXPathNodeSet("//volumes/volume", ctxt, &nodes)) < 0) + goto cleanup; + + for (i = 0; i < nnodes; i++) { + ctxt->node = nodes[i]; + + if (!(src = virStoragePoolSourceListNewSource(list))) + goto cleanup; + + if (!(src->dir = virXPathString("string(//name)", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to extract gluster volume name")); + goto cleanup; + } + + if (VIR_ALLOC_N(src->hosts, 1) < 0) + goto cleanup; + src->nhost = 1; + + if (VIR_STRDUP(src->hosts[0].name, host) < 0) + goto cleanup; + + src->format = pooltype; + } + + ret = nnodes; + + cleanup: + VIR_FREE(nodes); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + + return ret; +} + + /** * virStorageBackendFindGlusterPoolSources: * @host: host to detect volumes on @@ -2856,12 +2909,6 @@ virStorageBackendFindGlusterPoolSources(const char *host, char *glusterpath = NULL; char *outbuf = NULL; virCommandPtr cmd = NULL; - xmlDocPtr doc = NULL; - xmlXPathContextPtr ctxt = NULL; - xmlNodePtr *nodes = NULL; - virStoragePoolSource *src = NULL; - size_t i; - int nnodes; int rc; int ret = -1; @@ -2892,41 +2939,9 @@ virStorageBackendFindGlusterPoolSources(const char *host, goto cleanup; } - if (!(doc = virXMLParseStringCtxt(outbuf, _("(gluster_cli_output)"), - &ctxt))) - goto cleanup; - - if ((nnodes = virXPathNodeSet("//volumes/volume", ctxt, &nodes)) < 0) - goto cleanup; - - for (i = 0; i < nnodes; i++) { - ctxt->node = nodes[i]; - - if (!(src = virStoragePoolSourceListNewSource(list))) - goto cleanup; - - if (!(src->dir = virXPathString("string(//name)", ctxt))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to extract gluster volume name")); - goto cleanup; - } - - if (VIR_ALLOC_N(src->hosts, 1) < 0) - goto cleanup; - src->nhost = 1; - - if (VIR_STRDUP(src->hosts[0].name, host) < 0) - goto cleanup; - - src->format = pooltype; - } - - ret = nnodes; + ret = virStorageUtilGlusterExtractPoolSources(host, outbuf, pooltype, list); cleanup: - VIR_FREE(nodes); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(doc); VIR_FREE(outbuf); virCommandFree(cmd); VIR_FREE(glusterpath); diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index fa3b6522c..bf8424808 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -93,6 +93,10 @@ int virStorageBackendDeleteLocal(virConnectPtr conn, int virStorageBackendRefreshLocal(virConnectPtr conn, virStoragePoolObjPtr pool); +int virStorageUtilGlusterExtractPoolSources(const char *host, + const char *xml, + int pooltype, + virStoragePoolSourceListPtr list); int virStorageBackendFindGlusterPoolSources(const char *host, int pooltype, virStoragePoolSourceListPtr list, -- 2.12.1
Add a test program called virstorageutiltest and test the gluster pool detection code. --- tests/Makefile.am | 15 ++- .../virstorageutildata/gluster-parse-basic-dst.xml | 6 ++ .../virstorageutildata/gluster-parse-basic-src.xml | 47 ++++++++++ tests/virstorageutiltest.c | 102 +++++++++++++++++++++ 4 files changed, 167 insertions(+), 3 deletions(-) create mode 100644 tests/virstorageutildata/gluster-parse-basic-dst.xml create mode 100644 tests/virstorageutildata/gluster-parse-basic-src.xml create mode 100644 tests/virstorageutiltest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index a6f189b8b..254c4c0ad 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -160,7 +160,9 @@ EXTRA_DIST = \ xlconfigdata \ xmconfigdata \ xml2sexprdata \ - xml2vmxdata + xml2vmxdata \ + virstorageutildata \ + $(NULL) test_helpers = commandhelper ssh test_programs = virshtest sockettest \ @@ -352,7 +354,7 @@ test_programs += nwfilterxml2firewalltest endif WITH_NWFILTER if WITH_STORAGE -test_programs += storagevolxml2argvtest +test_programs += storagevolxml2argvtest virstorageutiltest endif WITH_STORAGE if WITH_STORAGE_FS @@ -859,6 +861,13 @@ genericxml2xmltest_LDADD = $(LDADDS) if WITH_STORAGE +virstorageutiltest_SOURCES = \ + virstorageutiltest.c testutils.c testutils.h +virstorageutiltest_LDADD = \ + ../src/libvirt_driver_storage_impl.la \ + $(LDADDS) \ + $(NULL) + storagevolxml2argvtest_SOURCES = \ storagevolxml2argvtest.c \ testutils.c testutils.h @@ -867,7 +876,7 @@ storagevolxml2argvtest_LDADD = \ ../src/libvirt_driver_storage_impl.la $(LDADDS) else ! WITH_STORAGE -EXTRA_DIST += storagevolxml2argvtest.c +EXTRA_DIST += storagevolxml2argvtest.c virstorageutiltest.c endif ! WITH_STORAGE storagevolxml2xmltest_SOURCES = \ diff --git a/tests/virstorageutildata/gluster-parse-basic-dst.xml b/tests/virstorageutildata/gluster-parse-basic-dst.xml new file mode 100644 index 000000000..fbde06f3b --- /dev/null +++ b/tests/virstorageutildata/gluster-parse-basic-dst.xml @@ -0,0 +1,6 @@ +<sources> + <source> + <host name='testhost'/> + <dir path='vol0'/> + </source> +</sources> diff --git a/tests/virstorageutildata/gluster-parse-basic-src.xml b/tests/virstorageutildata/gluster-parse-basic-src.xml new file mode 100644 index 000000000..08f97cb72 --- /dev/null +++ b/tests/virstorageutildata/gluster-parse-basic-src.xml @@ -0,0 +1,47 @@ +<?xml version="1.0" encoding="UTF-8" standalone="yes"?> +<cliOutput> + <opRet>0</opRet> + <opErrno>0</opErrno> + <opErrstr/> + <volInfo> + <volumes> + <volume> + <name>vol0</name> + <id>ac14dfa5-0b98-4593-a2aa-9fe2bb9b9ce3</id> + <status>1</status> + <statusStr>Started</statusStr> + <snapshotCount>0</snapshotCount> + <brickCount>2</brickCount> + <distCount>2</distCount> + <stripeCount>1</stripeCount> + <replicaCount>2</replicaCount> + <arbiterCount>0</arbiterCount> + <disperseCount>0</disperseCount> + <redundancyCount>0</redundancyCount> + <type>2</type> + <typeStr>Replicate</typeStr> + <transport>0</transport> + <bricks> + <brick uuid="a6f5ddea-bc6a-44db-ae1d-5aa1db743490">virt-gluster-node1:/bricks/brick1/brick<name>virt-gluster-node1:/bricks/brick1/brick</name><hostUuid>a6f5ddea-bc6a-44db-ae1d-5aa1db743490</hostUuid><isArbiter>0</isArbiter></brick> + <brick uuid="f4ab9fb1-44ec-443b-8783-e5f70ed78da3">virt-gluster-node2:/bricks/brick1/brick<name>virt-gluster-node2:/bricks/brick1/brick</name><hostUuid>f4ab9fb1-44ec-443b-8783-e5f70ed78da3</hostUuid><isArbiter>0</isArbiter></brick> + </bricks> + <optCount>3</optCount> + <options> + <option> + <name>nfs.disable</name> + <value>on</value> + </option> + <option> + <name>performance.readdir-ahead</name> + <value>on</value> + </option> + <option> + <name>transport.address-family</name> + <value>inet</value> + </option> + </options> + </volume> + <count>1</count> + </volumes> + </volInfo> +</cliOutput> diff --git a/tests/virstorageutiltest.c b/tests/virstorageutiltest.c new file mode 100644 index 000000000..0d4ede6c2 --- /dev/null +++ b/tests/virstorageutiltest.c @@ -0,0 +1,102 @@ +/* + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include <stdlib.h> + +#include "testutils.h" +#include "virerror.h" +#include "virfile.h" +#include "virlog.h" +#include "virstring.h" + +#include "storage/storage_util.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("tests.storageutiltest"); + + +static int +testGlusterLookupParse(const void *data) +{ + virStoragePoolSourceList list = { .type = VIR_STORAGE_POOL_GLUSTER, + .nsources = 0, + .sources = NULL + }; + const char *testname = data; + size_t i; + char *srcxml = NULL; + char *srcxmldata = NULL; + char *destxml = NULL; + char *actual = NULL; + int ret = -1; + + if (virAsprintf(&srcxml, + "%s/virstorageutildata/gluster-parse-%s-src.xml", + abs_srcdir, testname) < 0 || + virAsprintf(&destxml, + "%s/virstorageutildata/gluster-parse-%s-dst.xml", + abs_srcdir, testname) < 0) + goto cleanup; + + if (virTestLoadFile(srcxml, &srcxmldata) < 0) + goto cleanup; + + if (virStorageUtilGlusterExtractPoolSources("testhost", srcxmldata, + VIR_STORAGE_POOL_GLUSTER, + &list) < 0) + goto cleanup; + + if (!(actual = virStoragePoolSourceListFormat(&list))) + goto cleanup; + + ret = virTestCompareToFile(actual, destxml); + + cleanup: + VIR_FREE(srcxml); + VIR_FREE(srcxmldata); + VIR_FREE(destxml); + VIR_FREE(actual); + + for (i = 0; i < list.nsources; i++) + virStoragePoolSourceClear(&list.sources[i]); + VIR_FREE(list.sources); + + + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + +#define DO_TEST_GLUSTER_LOOKUP(testname) \ + if (virTestRun("gluster lookup " testname, testGlusterLookupParse, \ + testname) < 0) \ + ret = -1 + + DO_TEST_GLUSTER_LOOKUP("basic"); + +#undef DO_TEST_GLUSTER_LOOKUP + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 2.12.1
Use the relative lookup specifier rather than the global one. Otherwise only the first name would be looked up. Add a test case to cover the scenario. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1436574 --- src/storage/storage_util.c | 2 +- .../gluster-parse-multivol-dst.xml | 14 ++++++++++ .../gluster-parse-multivol-src.xml | 32 ++++++++++++++++++++++ tests/virstorageutiltest.c | 1 + 4 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 tests/virstorageutildata/gluster-parse-multivol-dst.xml create mode 100644 tests/virstorageutildata/gluster-parse-multivol-src.xml diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 8459e9d5b..e949fc3d4 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2862,7 +2862,7 @@ virStorageUtilGlusterExtractPoolSources(const char *host, if (!(src = virStoragePoolSourceListNewSource(list))) goto cleanup; - if (!(src->dir = virXPathString("string(//name)", ctxt))) { + if (!(src->dir = virXPathString("string(./name)", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to extract gluster volume name")); goto cleanup; diff --git a/tests/virstorageutildata/gluster-parse-multivol-dst.xml b/tests/virstorageutildata/gluster-parse-multivol-dst.xml new file mode 100644 index 000000000..d2d8fefc6 --- /dev/null +++ b/tests/virstorageutildata/gluster-parse-multivol-dst.xml @@ -0,0 +1,14 @@ +<sources> + <source> + <host name='testhost'/> + <dir path='aaa'/> + </source> + <source> + <host name='testhost'/> + <dir path='test'/> + </source> + <source> + <host name='testhost'/> + <dir path='test1'/> + </source> +</sources> diff --git a/tests/virstorageutildata/gluster-parse-multivol-src.xml b/tests/virstorageutildata/gluster-parse-multivol-src.xml new file mode 100644 index 000000000..0c1f9d10e --- /dev/null +++ b/tests/virstorageutildata/gluster-parse-multivol-src.xml @@ -0,0 +1,32 @@ +<?xml version="1.0" encoding="UTF-8" standalone="yes"?> +<!--- note that the XML file is truncated --> +<cliOutput> + <opRet>0</opRet> + <opErrno>0</opErrno> + <opErrstr/> + <volInfo> + <volumes> + <volume> + <name>aaa</name> + <id>d0b219d4-4169-4907-8994-d2e2434854ed</id> + <status>0</status> + <statusStr>Created</statusStr> + <snapshotCount>0</snapshotCount> + </volume> + <volume> + <name>test</name> + <id>32826068-2320-4b62-a825-2554edb7f020</id> + <status>1</status> + <statusStr>Started</statusStr> + <snapshotCount>0</snapshotCount> + </volume> + <volume> + <name>test1</name> + <id>dfa070f4-b12f-4166-8d68-041b73127abc</id> + <status>0</status> + <statusStr>Created</statusStr> + </volume> + <count>3</count> + </volumes> + </volInfo> +</cliOutput> diff --git a/tests/virstorageutiltest.c b/tests/virstorageutiltest.c index 0d4ede6c2..90f86233d 100644 --- a/tests/virstorageutiltest.c +++ b/tests/virstorageutiltest.c @@ -93,6 +93,7 @@ mymain(void) ret = -1 DO_TEST_GLUSTER_LOOKUP("basic"); + DO_TEST_GLUSTER_LOOKUP("multivol"); #undef DO_TEST_GLUSTER_LOOKUP -- 2.12.1
On Wed, Mar 29, 2017 at 15:23:37 +0200, Peter Krempa wrote:
Fix the broken XPath and add tests.
Peter Krempa (3): storage: util: Split out the gluster volume extraction code into new function test: Introduce testing of virStorageUtilGlusterExtractPoolSources storage: Fix XPath for looking up gluster volume name
I'll post another version of this since I found other problem in the code. I would require changing some code in this patch, so a reorder can be nicer.
participants (1)
-
Peter Krempa