[libvirt] [PATCH v2 0/5] Fix gluster pool lookup issues
Peter Krempa (5): storage: util: Add boolean differentiating between gluster lookup type 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 | 5 +- src/storage/storage_backend_gluster.c | 3 +- src/storage/storage_util.c | 110 ++++++++++++-------- src/storage/storage_util.h | 6 +- tests/Makefile.am | 15 ++- .../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, 330 insertions(+), 50 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.1
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 difference add a boolean to swithc between the types instead. --- src/storage/storage_backend_fs.c | 5 ++--- src/storage/storage_backend_gluster.c | 3 +-- src/storage/storage_util.c | 10 +++++++--- src/storage/storage_util.h | 2 +- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 1fc127a8c..a1b45e149 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -185,9 +185,8 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE retNFS = virStorageBackendFileSystemNetFindNFSPoolSources(&state); - retGluster = virStorageBackendFindGlusterPoolSources(state.host, - VIR_STORAGE_POOL_NETFS_GLUSTERFS, - &state.list, false); + retGluster = virStorageBackendFindGlusterPoolSources(state.host, &state.list, + true, false); if (retGluster < 0) goto cleanup; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 52c9ee372..cde7f9edb 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -513,8 +513,7 @@ virStorageBackendGlusterFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, } if ((rc = virStorageBackendFindGlusterPoolSources(source->hosts[0].name, - 0, /* currently ignored */ - &list, true)) < 0) + &list, false, true)) < 0) goto cleanup; if (rc == 0) { diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 7687eb89a..cad706199 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -2839,18 +2839,21 @@ virStorageBackendDeleteLocal(virConnectPtr conn ATTRIBUTE_UNUSED, /** * virStorageBackendFindGlusterPoolSources: * @host: host to detect volumes on - * @pooltype: src->format is set to this value * @list: list of storage pool sources to be filled + * @netfs: lookup will be used with netfs pools * @report: report error if the 'gluster' cli tool is missing * * Looks up gluster volumes on @host and fills them to @list. * + * If @netfs is specified the data is tweaked so that it can be used with netfs + * type pools. Otherwise the data is for use with native gluster pools. + * * Returns number of volumes on the host on success, or -1 on error. */ int virStorageBackendFindGlusterPoolSources(const char *host, - int pooltype, virStoragePoolSourceListPtr list, + bool netfs, bool report) { char *glusterpath = NULL; @@ -2918,7 +2921,8 @@ virStorageBackendFindGlusterPoolSources(const char *host, if (VIR_STRDUP(src->hosts[0].name, host) < 0) goto cleanup; - src->format = pooltype; + if (netfs) + src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; } ret = nnodes; diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index fa3b6522c..741baa78d 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -94,8 +94,8 @@ int virStorageBackendRefreshLocal(virConnectPtr conn, virStoragePoolObjPtr pool); int virStorageBackendFindGlusterPoolSources(const char *host, - int pooltype, virStoragePoolSourceListPtr list, + bool netfs, bool report); bool virStorageBackendDeviceIsEmpty(const char *devpath, -- 2.12.1
On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
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 difference add a boolean to swithc between the types instead.
s/difference/differences/ s/swithc/switch/ [...]
@@ -2839,18 +2839,21 @@ virStorageBackendDeleteLocal(virConnectPtr conn ATTRIBUTE_UNUSED, /** * virStorageBackendFindGlusterPoolSources: * @host: host to detect volumes on - * @pooltype: src->format is set to this value * @list: list of storage pool sources to be filled + * @netfs: lookup will be used with netfs pools * @report: report error if the 'gluster' cli tool is missing * * Looks up gluster volumes on @host and fills them to @list. * + * If @netfs is specified the data is tweaked so that it can be used with netfs + * type pools. Otherwise the data is for use with native gluster pools. + * * Returns number of volumes on the host on success, or -1 on error. */ int virStorageBackendFindGlusterPoolSources(const char *host, - int pooltype, virStoragePoolSourceListPtr list, + bool netfs,
I suggest using virStoragePoolType instead of bool here, eg. callers will pass either VIR_STORAGE_POOL_GLUSTER for native gluster pools or VIR_STORAGE_POOL_NETFS for netfs pools. Passing any other value in the enumeration would of course result in an error. This would make the calling sites less opaque. [...]
@@ -2918,7 +2921,8 @@ virStorageBackendFindGlusterPoolSources(const char *host, if (VIR_STRDUP(src->hosts[0].name, host) < 0) goto cleanup; - src->format = pooltype; + if (netfs) + src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS;
In patch 5/5 you're going to move this chunk of code earlier in the function while changing it: I suggest you move it in this patch instead, so there's only a single code motion instead of two. ACK with the above suggestions addressed, or if you manage to convince me that they're best left unaddressed :) -- Andrea Bolognani / Red Hat / Virtualization
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 cad706199..3b55bafc0 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, + bool netfs) +{ + 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; + + if (netfs) + src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + } + + ret = nnodes; + + cleanup: + VIR_FREE(nodes); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + + return ret; +} + + /** * virStorageBackendFindGlusterPoolSources: * @host: host to detect volumes on @@ -2859,12 +2913,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; @@ -2895,42 +2943,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; - - if (netfs) - src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; - } - - ret = nnodes; + ret = virStorageUtilGlusterExtractPoolSources(host, outbuf, list, netfs); 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 741baa78d..ecd67e2fc 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, + bool netfs); int virStorageBackendFindGlusterPoolSources(const char *host, virStoragePoolSourceListPtr list, bool netfs, -- 2.12.1
On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote: [...]
@@ -93,6 +93,10 @@ int virStorageBackendDeleteLocal(virConnectPtr conn, int virStorageBackendRefreshLocal(virConnectPtr conn, virStoragePoolObjPtr pool); +int virStorageUtilGlusterExtractPoolSources(const char *host, + const char *xml, + virStoragePoolSourceListPtr list, + bool netfs);
Please add a comment along the lines of "For test suite use only" here. Ideally we'd have a separate *priv.h header file to be used for the purpose, but that's out of scope. ACK -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Apr 03, 2017 at 16:20:26 +0200, Andrea Bolognani wrote:
On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote: [...]
@@ -93,6 +93,10 @@ int virStorageBackendDeleteLocal(virConnectPtr conn, int virStorageBackendRefreshLocal(virConnectPtr conn, virStoragePoolObjPtr pool); +int virStorageUtilGlusterExtractPoolSources(const char *host, + const char *xml, + virStoragePoolSourceListPtr list, + bool netfs);
Please add a comment along the lines of "For test suite use only" here. Ideally we'd have a separate *priv.h header file to be used for the purpose, but that's out of scope.
Why? This function can be used wherever it's necessary. It's by no-means specific to the test suite.
On Mon, 2017-04-03 at 16:38 +0200, Peter Krempa wrote:
+int virStorageUtilGlusterExtractPoolSources(const char *host, + const char *xml, + virStoragePoolSourceListPtr list, + bool netfs); Please add a comment along the lines of "For test suite use only" here. Ideally we'd have a separate *priv.h header file to be used for the purpose, but that's out of scope. Why? This function can be used wherever it's necessary. It's by no-means specific to the test suite.
It would probably be a static function, or would never have been ripped out of virStorageBackendFindGlusterPoolSources() to begin with, were not for the test suite, so I'd say the comment would be warranted until a non-theoretical use for it is found outside of the test suite. If you don't feel the same way, you can safely disregard my remark though. -- Andrea Bolognani / Red Hat / Virtualization
Add a test program called virstorageutiltest and test the gluster pool detection code. --- tests/Makefile.am | 15 ++- .../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, 184 insertions(+), 3 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/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-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..b13ecb5ab --- /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; + bool netfs; + int type; +}; + +static int +testGlusterLookupParse(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->netfs) < 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_LOOKUP_FULL(testname, sffx, testnetfs, 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.netfs = testnetfs; \ + data.type = pooltype; \ + if (virTestRun("gluster lookup " sffx " " testname, \ + testGlusterLookupParse, &data) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_GLUSTER_LOOKUP_NATIVE(testname) \ + DO_TEST_GLUSTER_LOOKUP_FULL(testname, "native", false, VIR_STORAGE_POOL_GLUSTER) +#define DO_TEST_GLUSTER_LOOKUP_NETFS(testname) \ + DO_TEST_GLUSTER_LOOKUP_FULL(testname, "netfs", true, VIR_STORAGE_POOL_NETFS) + + DO_TEST_GLUSTER_LOOKUP_NATIVE("basic"); + DO_TEST_GLUSTER_LOOKUP_NETFS("basic"); + +#undef DO_TEST_GLUSTER_LOOKUP_NATIVE +#undef DO_TEST_GLUSTER_LOOKUP_NETFS +#undef DO_TEST_GLUSTER_LOOKUP_FULL + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 2.12.1
On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote: [...]
@@ -352,7 +354,7 @@ test_programs += nwfilterxml2firewalltest endif WITH_NWFILTER if WITH_STORAGE -test_programs += storagevolxml2argvtest +test_programs += storagevolxml2argvtest virstorageutiltest
Since you have to touch these lines regardless, please turn this... [...]
@@ -859,6 +861,13 @@ genericxml2xmltest_LDADD = $(LDADDS) if WITH_STORAGE +virstorageutiltest_SOURCES = \ + virstorageutiltest.c testutils.c testutils.h
... and this ...
+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
... and finally this into proper, one-file-per-line, $(NULL)-terminated file lists as a courtesy to the next developer :) You should also be able to use $(qemu_LDADDS) instead of spelling out ../src/libvirt_driver_storage_impl.la above. [...]
+ <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>
Does Gluster's <brick> element really contain a bare text node followed by a bunch of other elements? Ewww. [...]
+#define DO_TEST_GLUSTER_LOOKUP_FULL(testname, sffx, testnetfs, 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"; \
There's a spurious space between sffx and ".xml".
+ data.netfs = testnetfs; \ + data.type = pooltype; \ + if (virTestRun("gluster lookup " sffx " " testname, \ + testGlusterLookupParse, &data) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_GLUSTER_LOOKUP_NATIVE(testname) \ + DO_TEST_GLUSTER_LOOKUP_FULL(testname, "native", false, VIR_STORAGE_POOL_GLUSTER) +#define DO_TEST_GLUSTER_LOOKUP_NETFS(testname) \ + DO_TEST_GLUSTER_LOOKUP_FULL(testname, "netfs", true, VIR_STORAGE_POOL_NETFS) + + DO_TEST_GLUSTER_LOOKUP_NATIVE("basic"); + DO_TEST_GLUSTER_LOOKUP_NETFS("basic");
Between GLUSTER_LOOKUP, gluster-parse and GlusterLookupParse the naming is a bit all over the place. Do you think you could make it more consistent? Especially since none of those seems to have any direct connection with virStorageUtilGlusterExtractPoolSources(), the API being tested. [...]
+#undef DO_TEST_GLUSTER_LOOKUP_NATIVE +#undef DO_TEST_GLUSTER_LOOKUP_NETFS +#undef DO_TEST_GLUSTER_LOOKUP_FULL
The #undefs are a bit unnecessary in this context, I'd leave them out. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Apr 03, 2017 at 17:03:43 +0200, Andrea Bolognani wrote:
On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
[...]
+virstorageutiltest_LDADD = \ + ../src/libvirt_driver_storage_impl.la \ + $(LDADDS) \ + $(NULL) +
[...]
You should also be able to use $(qemu_LDADDS) instead of spelling out ../src/libvirt_driver_storage_impl.la above.
Yes, but that pulls in all the qemu driver crap, which has no place in a test testing only the storage driver. [...]
+#undef DO_TEST_GLUSTER_LOOKUP_NATIVE +#undef DO_TEST_GLUSTER_LOOKUP_NETFS +#undef DO_TEST_GLUSTER_LOOKUP_FULL
The #undefs are a bit unnecessary in this context, I'd leave them out.
It's true that it would be hard to use these somewhere else (since they are in a .c file) but ad-hoc macros should be undefined. Otherwise they are not ad-hoc and should bear the VIR_ prefix.
On Tue, 2017-04-04 at 13:50 +0200, Peter Krempa wrote:
+#undef DO_TEST_GLUSTER_LOOKUP_NATIVE +#undef DO_TEST_GLUSTER_LOOKUP_NETFS +#undef DO_TEST_GLUSTER_LOOKUP_FULL
The #undefs are a bit unnecessary in this context, I'd leave them out.
It's true that it would be hard to use these somewhere else (since they are in a .c file) but ad-hoc macros should be undefined. Otherwise they are not ad-hoc and should bear the VIR_ prefix.
Feel free to leave the hunk in then :) -- 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 3b55bafc0..19c73497f 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 b13ecb5ab..37a3524e6 100644 --- a/tests/virstorageutiltest.c +++ b/tests/virstorageutiltest.c @@ -101,6 +101,8 @@ mymain(void) DO_TEST_GLUSTER_LOOKUP_NATIVE("basic"); DO_TEST_GLUSTER_LOOKUP_NETFS("basic"); + DO_TEST_GLUSTER_LOOKUP_NATIVE("multivol"); + DO_TEST_GLUSTER_LOOKUP_NETFS("multivol"); #undef DO_TEST_GLUSTER_LOOKUP_NATIVE #undef DO_TEST_GLUSTER_LOOKUP_NETFS -- 2.12.1
On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote:
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
ACK -- Andrea Bolognani / Red Hat / Virtualization
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 | 15 ++++++++++++--- tests/virstorageutildata/gluster-parse-basic-native.xml | 3 ++- .../virstorageutildata/gluster-parse-multivol-native.xml | 9 ++++++--- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 19c73497f..241b6890a 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,12 +2863,22 @@ 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 (netfs) { + src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + src->dir = volname; + } else { + src->name = volname; + + if (VIR_STRDUP(src->dir, "/") < 0) + goto cleanup; + } + if (VIR_ALLOC_N(src->hosts, 1) < 0) goto cleanup; src->nhost = 1; @@ -2875,8 +2886,6 @@ virStorageUtilGlusterExtractPoolSources(const char *host, if (VIR_STRDUP(src->hosts[0].name, host) < 0) goto cleanup; - if (netfs) - src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; } ret = nnodes; 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.1
On Thu, 2017-03-30 at 17:12 +0200, Peter Krempa wrote: [...]
+ if (netfs) { + src->format = VIR_STORAGE_POOL_NETFS_GLUSTERFS; + src->dir = volname; + } else { + src->name = volname; + + if (VIR_STRDUP(src->dir, "/") < 0) + goto cleanup;
I'll admit I didn't dig, but is hardcoding "/" here the right thing to do? Assuming it is because some other code takes care of setting it to a different value when needed, ACK -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Peter Krempa