[libvirt] [PATCH v3 0/5] Fix gluster pool lookup issues
Naming, style and other fixes suggested by Andrea in his review. The changes were too invasive so I'll rather repost it. Peter Krempa (5): storage: util: Pass pool type to virStorageBackendFindGlusterPoolSources 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 storage: gluster: Use volume name as "<name>" field in the XML src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_util.c | 114 +++++++++++++-------- src/storage/storage_util.h | 6 +- tests/Makefile.am | 16 ++- .../gluster-parse-basic-native.xml | 7 ++ .../gluster-parse-basic-netfs.xml | 7 ++ .../virstorageutildata/gluster-parse-basic-src.xml | 47 +++++++++ .../gluster-parse-multivol-native.xml | 17 +++ .../gluster-parse-multivol-netfs.xml | 17 +++ .../gluster-parse-multivol-src.xml | 32 ++++++ tests/virstorageutiltest.c | 114 +++++++++++++++++++++ 12 files changed, 336 insertions(+), 45 deletions(-) create mode 100644 tests/virstorageutildata/gluster-parse-basic-native.xml create mode 100644 tests/virstorageutildata/gluster-parse-basic-netfs.xml create mode 100644 tests/virstorageutildata/gluster-parse-basic-src.xml create mode 100644 tests/virstorageutildata/gluster-parse-multivol-native.xml create mode 100644 tests/virstorageutildata/gluster-parse-multivol-netfs.xml create mode 100644 tests/virstorageutildata/gluster-parse-multivol-src.xml create mode 100644 tests/virstorageutiltest.c -- 2.12.2
The native gluster pool source list data differs from the data used for attaching gluster volumes as netfs pools. Currently the only difference was the format. Since native pools don't use it and later there will be more differences add a more deterministic way to switch between the types instead. --- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_util.c | 13 +++++++++---- src/storage/storage_util.h | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 1fc127a8c..bf1d7de43 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -186,7 +186,7 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE retNFS = virStorageBackendFileSystemNetFindNFSPoolSources(&state); retGluster = virStorageBackendFindGlusterPoolSources(state.host, - VIR_STORAGE_POOL_NETFS_GLUSTERFS, + VIR_STORAGE_POOL_NETFS, &state.list, false); if (retGluster < 0) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 52c9ee372..30a41369a 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -513,7 +513,7 @@ virStorageBackendGlusterFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, } if ((rc = virStorageBackendFindGlusterPoolSources(source->hosts[0].name, - 0, /* currently ignored */ + VIR_STORAGE_POOL_GLUSTER, &list, true)) < 0) goto cleanup; diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 0ceaab6b9..715361923 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2839,17 +2839,21 @@ virStorageBackendDeleteLocal(virConnectPtr conn ATTRIBUTE_UNUSED, /** * virStorageBackendFindGlusterPoolSources: * @host: host to detect volumes on - * @pooltype: src->format is set to this value + * @pooltype: type of the pool * @list: list of storage pool sources to be filled * @report: report error if the 'gluster' cli tool is missing * * Looks up gluster volumes on @host and fills them to @list. * + * @pooltype allows to influence the specific differences between netfs and + * native gluster pools. Users should pass only VIR_STORAGE_POOL_NETFS or + * VIR_STORAGE_POOL_GLUSTER. + * * Returns number of volumes on the host on success, or -1 on error. */ int virStorageBackendFindGlusterPoolSources(const char *host, - int pooltype, + virStoragePoolType pooltype, virStoragePoolSourceListPtr list, bool report) { @@ -2911,14 +2915,15 @@ virStorageBackendFindGlusterPoolSources(const char *host, goto cleanup; } + if (pooltype == VIR_STORAGE_POOL_NETFS) + src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + 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; diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index fa3b6522c..1ba259c1e 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -94,7 +94,7 @@ int virStorageBackendRefreshLocal(virConnectPtr conn, virStoragePoolObjPtr pool); int virStorageBackendFindGlusterPoolSources(const char *host, - int pooltype, + virStoragePoolType pooltype, virStoragePoolSourceListPtr list, bool report); -- 2.12.2
To allow testing of the algorithm, split out the extractor into a separate helper. --- src/storage/storage_util.c | 95 +++++++++++++++++++++++++++------------------- src/storage/storage_util.h | 4 ++ 2 files changed, 59 insertions(+), 40 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 715361923..b7c594da2 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2836,6 +2836,60 @@ virStorageBackendDeleteLocal(virConnectPtr conn ATTRIBUTE_UNUSED, } +int +virStorageUtilGlusterExtractPoolSources(const char *host, + const char *xml, + virStoragePoolSourceListPtr list, + virStoragePoolType pooltype) +{ + 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 (pooltype == VIR_STORAGE_POOL_NETFS) + src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + + if (VIR_ALLOC_N(src->hosts, 1) < 0) + goto cleanup; + src->nhost = 1; + + if (VIR_STRDUP(src->hosts[0].name, host) < 0) + goto cleanup; + } + + ret = nnodes; + + cleanup: + VIR_FREE(nodes); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + + return ret; +} + + /** * virStorageBackendFindGlusterPoolSources: * @host: host to detect volumes on @@ -2860,12 +2914,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; @@ -2896,42 +2944,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 (pooltype == VIR_STORAGE_POOL_NETFS) - src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; - - if (VIR_ALLOC_N(src->hosts, 1) < 0) - goto cleanup; - src->nhost = 1; - - if (VIR_STRDUP(src->hosts[0].name, host) < 0) - goto cleanup; - } - - ret = nnodes; + ret = virStorageUtilGlusterExtractPoolSources(host, outbuf, list, pooltype); 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 1ba259c1e..602d3a069 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, + virStoragePoolSourceListPtr list, + virStoragePoolType pooltype); int virStorageBackendFindGlusterPoolSources(const char *host, virStoragePoolType pooltype, virStoragePoolSourceListPtr list, -- 2.12.2
Add a test program called virstorageutiltest and test the gluster pool detection code. --- tests/Makefile.am | 16 ++- .../gluster-parse-basic-native.xml | 6 ++ .../gluster-parse-basic-netfs.xml | 7 ++ .../virstorageutildata/gluster-parse-basic-src.xml | 47 +++++++++ tests/virstorageutiltest.c | 112 +++++++++++++++++++++ 5 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 tests/virstorageutildata/gluster-parse-basic-native.xml create mode 100644 tests/virstorageutildata/gluster-parse-basic-netfs.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..aa9d2eb3a 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 \ @@ -353,6 +355,7 @@ endif WITH_NWFILTER if WITH_STORAGE test_programs += storagevolxml2argvtest +test_programs += virstorageutiltest endif WITH_STORAGE if WITH_STORAGE_FS @@ -859,6 +862,16 @@ genericxml2xmltest_LDADD = $(LDADDS) if WITH_STORAGE +virstorageutiltest_SOURCES = \ + virstorageutiltest.c \ + testutils.c \ + testutils.h \ + $(NULL) +virstorageutiltest_LDADD = \ + ../src/libvirt_driver_storage_impl.la \ + $(LDADDS) \ + $(NULL) + storagevolxml2argvtest_SOURCES = \ storagevolxml2argvtest.c \ testutils.c testutils.h @@ -868,6 +881,7 @@ storagevolxml2argvtest_LDADD = \ else ! WITH_STORAGE EXTRA_DIST += storagevolxml2argvtest.c +EXTRA_DIST += virstorageutiltest.c endif ! WITH_STORAGE storagevolxml2xmltest_SOURCES = \ diff --git a/tests/virstorageutildata/gluster-parse-basic-native.xml b/tests/virstorageutildata/gluster-parse-basic-native.xml new file mode 100644 index 000000000..fbde06f3b --- /dev/null +++ b/tests/virstorageutildata/gluster-parse-basic-native.xml @@ -0,0 +1,6 @@ +<sources> + <source> + <host name='testhost'/> + <dir path='vol0'/> + </source> +</sources> diff --git a/tests/virstorageutildata/gluster-parse-basic-netfs.xml b/tests/virstorageutildata/gluster-parse-basic-netfs.xml new file mode 100644 index 000000000..8aadd50a1 --- /dev/null +++ b/tests/virstorageutildata/gluster-parse-basic-netfs.xml @@ -0,0 +1,7 @@ +<sources> + <source> + <host name='testhost'/> + <dir path='vol0'/> + <format type='glusterfs'/> + </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..12597a014 --- /dev/null +++ b/tests/virstorageutiltest.c @@ -0,0 +1,112 @@ +/* + * 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"); + + +struct testGlusterLookupParseData { + const char *srcxml; + const char *dstxml; + int type; +}; + +static int +testGlusterExtractPoolSources(const void *opaque) +{ + const struct testGlusterLookupParseData *data = opaque; + virStoragePoolSourceList list = { .type = data->type, + .nsources = 0, + .sources = NULL + }; + size_t i; + char *srcxmldata = NULL; + char *actual = NULL; + int ret = -1; + + if (virTestLoadFile(data->srcxml, &srcxmldata) < 0) + goto cleanup; + + if (virStorageUtilGlusterExtractPoolSources("testhost", srcxmldata, + &list, data->type) < 0) + goto cleanup; + + if (!(actual = virStoragePoolSourceListFormat(&list))) + goto cleanup; + + ret = virTestCompareToFile(actual, data->dstxml); + + cleanup: + VIR_FREE(srcxmldata); + 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_EXTRACT_POOL_SOURCES_FULL(testname, sffx, pooltype) \ + do { \ + struct testGlusterLookupParseData data; \ + data.srcxml = abs_srcdir "/virstorageutildata/" \ + "gluster-parse-" testname "-src.xml"; \ + data.dstxml = abs_srcdir "/virstorageutildata/" \ + "gluster-parse-" testname "-" sffx ".xml"; \ + data.type = pooltype; \ + if (virTestRun("gluster lookup " sffx " " testname, \ + testGlusterExtractPoolSources, &data) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NATIVE(testname) \ + DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_FULL(testname, "native", \ + VIR_STORAGE_POOL_GLUSTER) +#define DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NETFS(testname) \ + DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_FULL(testname, "netfs", \ + VIR_STORAGE_POOL_NETFS) + + DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NATIVE("basic"); + DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NETFS("basic"); + +#undef DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NATIVE +#undef DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NETFS +#undef DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_FULL + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 2.12.2
On Tue, 2017-04-04 at 14:20 +0200, Peter Krempa wrote: [...]
+struct testGlusterLookupParseData {
Now that the test function has been renamed, the data struct should be renamed to match as well.
+ const char *srcxml; + const char *dstxml; + int type;
This should be virStoragePoolType rather than int. [...]
+#define DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_FULL(testname, sffx, pooltype) \ + do { \ + struct testGlusterLookupParseData data; \ + data.srcxml = abs_srcdir "/virstorageutildata/" \ + "gluster-parse-" testname "-src.xml"; \ + data.dstxml = abs_srcdir "/virstorageutildata/" \ + "gluster-parse-" testname "-" sffx ".xml"; \ + data.type = pooltype; \ + if (virTestRun("gluster lookup " sffx " " testname, \ + testGlusterExtractPoolSources, &data) < 0) \ + ret = -1; \ + } while (0)
Input file name and verbose test output are still inconsistent between each other, with the name of the test function and with the name of the test macro. FWIW I think "gluster-parse" is descriptive enough and has the advantage of being short, but I don't particularly care what name you pick as long as you stick with it in earnest afterwards :) ACK with that fixed. -- Andrea Bolognani / Red Hat / Virtualization
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-native.xml | 14 ++++++++++ .../gluster-parse-multivol-netfs.xml | 17 ++++++++++++ .../gluster-parse-multivol-src.xml | 32 ++++++++++++++++++++++ tests/virstorageutiltest.c | 2 ++ 5 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 tests/virstorageutildata/gluster-parse-multivol-native.xml create mode 100644 tests/virstorageutildata/gluster-parse-multivol-netfs.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 b7c594da2..1e44a2da4 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-native.xml b/tests/virstorageutildata/gluster-parse-multivol-native.xml new file mode 100644 index 000000000..d2d8fefc6 --- /dev/null +++ b/tests/virstorageutildata/gluster-parse-multivol-native.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-netfs.xml b/tests/virstorageutildata/gluster-parse-multivol-netfs.xml new file mode 100644 index 000000000..3a542999d --- /dev/null +++ b/tests/virstorageutildata/gluster-parse-multivol-netfs.xml @@ -0,0 +1,17 @@ +<sources> + <source> + <host name='testhost'/> + <dir path='aaa'/> + <format type='glusterfs'/> + </source> + <source> + <host name='testhost'/> + <dir path='test'/> + <format type='glusterfs'/> + </source> + <source> + <host name='testhost'/> + <dir path='test1'/> + <format type='glusterfs'/> + </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 12597a014..932c922f2 100644 --- a/tests/virstorageutiltest.c +++ b/tests/virstorageutiltest.c @@ -101,6 +101,8 @@ mymain(void) DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NATIVE("basic"); DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NETFS("basic"); + DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NATIVE("multivol"); + DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NETFS("multivol"); #undef DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NATIVE #undef DO_TEST_GLUSTER_EXTRACT_POOL_SOURCES_NETFS -- 2.12.2
For native gluster pools the <dir> field denotes a directory inside the pool. For the actual pool name the <name> field has to be used. --- src/storage/storage_util.c | 16 ++++++++++++++-- tests/virstorageutildata/gluster-parse-basic-native.xml | 3 ++- .../virstorageutildata/gluster-parse-multivol-native.xml | 9 ++++++--- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 1e44a2da4..7cc125a38 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2846,6 +2846,7 @@ virStorageUtilGlusterExtractPoolSources(const char *host, xmlXPathContextPtr ctxt = NULL; xmlNodePtr *nodes = NULL; virStoragePoolSource *src = NULL; + char *volname; size_t i; int nnodes; int ret = -1; @@ -2862,14 +2863,25 @@ virStorageUtilGlusterExtractPoolSources(const char *host, if (!(src = virStoragePoolSourceListNewSource(list))) goto cleanup; - if (!(src->dir = virXPathString("string(./name)", ctxt))) { + if (!(volname = virXPathString("string(./name)", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to extract gluster volume name")); goto cleanup; } - if (pooltype == VIR_STORAGE_POOL_NETFS) + if (pooltype == VIR_STORAGE_POOL_NETFS) { src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + src->dir = volname; + } else if (pooltype == VIR_STORAGE_POOL_GLUSTER) { + src->name = volname; + + if (VIR_STRDUP(src->dir, "/") < 0) + goto cleanup; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unsupported gluster lookup")); + goto cleanup; + } if (VIR_ALLOC_N(src->hosts, 1) < 0) goto cleanup; diff --git a/tests/virstorageutildata/gluster-parse-basic-native.xml b/tests/virstorageutildata/gluster-parse-basic-native.xml index fbde06f3b..895d0f3fd 100644 --- a/tests/virstorageutildata/gluster-parse-basic-native.xml +++ b/tests/virstorageutildata/gluster-parse-basic-native.xml @@ -1,6 +1,7 @@ <sources> <source> <host name='testhost'/> - <dir path='vol0'/> + <dir path='/'/> + <name>vol0</name> </source> </sources> diff --git a/tests/virstorageutildata/gluster-parse-multivol-native.xml b/tests/virstorageutildata/gluster-parse-multivol-native.xml index d2d8fefc6..c758ac5aa 100644 --- a/tests/virstorageutildata/gluster-parse-multivol-native.xml +++ b/tests/virstorageutildata/gluster-parse-multivol-native.xml @@ -1,14 +1,17 @@ <sources> <source> <host name='testhost'/> - <dir path='aaa'/> + <dir path='/'/> + <name>aaa</name> </source> <source> <host name='testhost'/> - <dir path='test'/> + <dir path='/'/> + <name>test</name> </source> <source> <host name='testhost'/> - <dir path='test1'/> + <dir path='/'/> + <name>test1</name> </source> </sources> -- 2.12.2
On 04/04/2017 08:20 AM, Peter Krempa wrote:
For native gluster pools the <dir> field denotes a directory inside the pool. For the actual pool name the <name> field has to be used. --- src/storage/storage_util.c | 16 ++++++++++++++-- tests/virstorageutildata/gluster-parse-basic-native.xml | 3 ++- .../virstorageutildata/gluster-parse-multivol-native.xml | 9 ++++++--- 3 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 1e44a2da4..7cc125a38 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2846,6 +2846,7 @@ virStorageUtilGlusterExtractPoolSources(const char *host, xmlXPathContextPtr ctxt = NULL; xmlNodePtr *nodes = NULL; virStoragePoolSource *src = NULL; + char *volname; size_t i; int nnodes; int ret = -1; @@ -2862,14 +2863,25 @@ virStorageUtilGlusterExtractPoolSources(const char *host, if (!(src = virStoragePoolSourceListNewSource(list))) goto cleanup;
- if (!(src->dir = virXPathString("string(./name)", ctxt))) { + if (!(volname = virXPathString("string(./name)", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to extract gluster volume name")); goto cleanup; }
- if (pooltype == VIR_STORAGE_POOL_NETFS) + if (pooltype == VIR_STORAGE_POOL_NETFS) { src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + src->dir = volname; + } else if (pooltype == VIR_STORAGE_POOL_GLUSTER) { + src->name = volname; + + if (VIR_STRDUP(src->dir, "/") < 0) + goto cleanup; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unsupported gluster lookup")); + goto cleanup;
Coverity gleefully tells me volname is leaked this morning... John
+ }
if (VIR_ALLOC_N(src->hosts, 1) < 0) goto cleanup; [...]
On Wed, Apr 05, 2017 at 07:03:39 -0400, John Ferlan wrote:
On 04/04/2017 08:20 AM, Peter Krempa wrote:
For native gluster pools the <dir> field denotes a directory inside the pool. For the actual pool name the <name> field has to be used. --- src/storage/storage_util.c | 16 ++++++++++++++-- tests/virstorageutildata/gluster-parse-basic-native.xml | 3 ++- .../virstorageutildata/gluster-parse-multivol-native.xml | 9 ++++++--- 3 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 1e44a2da4..7cc125a38 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2846,6 +2846,7 @@ virStorageUtilGlusterExtractPoolSources(const char *host, xmlXPathContextPtr ctxt = NULL; xmlNodePtr *nodes = NULL; virStoragePoolSource *src = NULL; + char *volname; size_t i; int nnodes; int ret = -1; @@ -2862,14 +2863,25 @@ virStorageUtilGlusterExtractPoolSources(const char *host, if (!(src = virStoragePoolSourceListNewSource(list))) goto cleanup;
- if (!(src->dir = virXPathString("string(./name)", ctxt))) { + if (!(volname = virXPathString("string(./name)", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to extract gluster volume name")); goto cleanup; }
- if (pooltype == VIR_STORAGE_POOL_NETFS) + if (pooltype == VIR_STORAGE_POOL_NETFS) { src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + src->dir = volname; + } else if (pooltype == VIR_STORAGE_POOL_GLUSTER) { + src->name = volname; + + if (VIR_STRDUP(src->dir, "/") < 0) + goto cleanup; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unsupported gluster lookup")); + goto cleanup;
Coverity gleefully tells me volname is leaked this morning...
Sigh. I blame Andrea for his idea of passing pooltype, which resulted into this. It won't ever happen since the else section is dead code, but I'll add the free in this case since it's an actual bug.
On Tue, 2017-04-04 at 14:20 +0200, Peter Krempa wrote:
Naming, style and other fixes suggested by Andrea in his review. The changes were too invasive so I'll rather repost it.
ACK series if you take care of the comments to patch 3/5. Another respin should not be necessary. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Apr 04, 2017 at 16:09:11 +0200, Andrea Bolognani wrote:
On Tue, 2017-04-04 at 14:20 +0200, Peter Krempa wrote:
Naming, style and other fixes suggested by Andrea in his review. The changes were too invasive so I'll rather repost it.
ACK series if you take care of the comments to patch 3/5. Another respin should not be necessary.
Thanks, I've fixed those issues and pushed this series.
participants (3)
-
Andrea Bolognani -
John Ferlan -
Peter Krempa