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

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

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

On 04/04/2017 08:20 AM, Peter Krempa wrote: Coverity gleefully tells me volname is leaked this morning... John [...]