[libvirt] [PATCH 0/8] Add tests for Storage Pool startup command line generation
Similar to qemuxml2argv and storagevolxml2argv, add storagepoolxml2argvtest in order to check the command line creation for the pool 'Start' commands. Only applicable for pool types with a "@startPool" function - that is disk, fs, iscsi, logical, scsi, and vstorage and further restricted if the pool doesn't use virCommandPtr type processing. This initial series only addresses fs and logical so that it's not "too many patches to review". The iscsi and vstorage pools could have their own tests, with the iscsi ones being more challenging to write. The disk pool does not have the normal command line processing to start something - rather the command line processing is used to validate that the pool about to be started is of a valid type based on the label seen at startup compared to the pool XML. This processing is not easily mocked. The scsi (for NPIV) doesn't use the virCommandPtr style interfaces, but at least that processing is tested in other ways using testCreateVport from test_driver.c. John Ferlan (8): storage: Extract out mount command creation for FS Backend storage: Move FS backend mount creation command helper storage: Move virStorageBackendFileSystemGetPoolSource tests: Introduce tests for storage pool xml to argv checks tests: Add storagepool xml test for netfs-auto storage: Rework virStorageBackendFileSystemMountCmd logical: Fix @on argument type storage: Add tests for logical backend startup src/storage/storage_backend_fs.c | 77 +------- src/storage/storage_backend_logical.c | 12 +- src/storage/storage_util.c | 122 ++++++++++++ src/storage/storage_util.h | 11 ++ tests/Makefile.am | 12 ++ tests/storagepoolxml2argvdata/pool-fs.argv | 1 + .../pool-logical-create.argv | 1 + .../pool-logical-noname.argv | 1 + .../pool-logical-nopath.argv | 1 + .../storagepoolxml2argvdata/pool-logical.argv | 1 + .../pool-netfs-auto.argv | 1 + .../pool-netfs-cifs.argv | 1 + .../pool-netfs-gluster.argv | 1 + tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + tests/storagepoolxml2argvtest.c | 175 ++++++++++++++++++ .../storagepoolxml2xmlin/pool-netfs-auto.xml | 19 ++ .../storagepoolxml2xmlout/pool-netfs-auto.xml | 20 ++ tests/storagepoolxml2xmltest.c | 1 + 18 files changed, 375 insertions(+), 83 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-create.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-noname.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-nopath.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-auto.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv create mode 100644 tests/storagepoolxml2argvtest.c create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-auto.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-auto.xml -- 2.17.2
Extract out the code that is used to create the MOUNT command for starting the pool. We can use this for Storage Pool XML to Argv testing to ensure code changes don't alter how a storage pool is started. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 70 +++++++++++++++++++------------- 1 file changed, 41 insertions(+), 29 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0837443391..4bf411b330 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -330,19 +330,11 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) return ret; } -/** - * @pool storage pool to mount - * - * Ensure that a FS storage pool is mounted on its target location. - * If already mounted, this is a no-op - * - * Returns 0 if successfully mounted, -1 on error - */ -static int -virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) + +static virCommandPtr +virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, + const char *src) { - virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *src = NULL; /* 'mount -t auto' doesn't seem to auto determine nfs (or cifs), * while plain 'mount' does. We have to craft separate argvs to * accommodate this */ @@ -353,23 +345,6 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) bool cifsfs = (def->type == VIR_STORAGE_POOL_NETFS && def->source.format == VIR_STORAGE_POOL_NETFS_CIFS); virCommandPtr cmd = NULL; - int ret = -1; - int rc; - - if (virStorageBackendFileSystemIsValid(pool) < 0) - return -1; - - if ((rc = virStorageBackendFileSystemIsMounted(pool)) < 0) - return -1; - - /* Short-circuit if already mounted */ - if (rc == 1) { - VIR_INFO("Target '%s' is already mounted", def->target.path); - return 0; - } - - if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) - return -1; if (netauto) cmd = virCommandNewArgList(MOUNT, @@ -404,6 +379,43 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) def->target.path, NULL); + return cmd; +} + + +/** + * @pool storage pool to mount + * + * Ensure that a FS storage pool is mounted on its target location. + * If already mounted, this is a no-op + * + * Returns 0 if successfully mounted, -1 on error + */ +static int +virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) +{ + virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); + char *src = NULL; + virCommandPtr cmd = NULL; + int ret = -1; + int rc; + + if (virStorageBackendFileSystemIsValid(pool) < 0) + return -1; + + if ((rc = virStorageBackendFileSystemIsMounted(pool)) < 0) + return -1; + + /* Short-circuit if already mounted */ + if (rc == 1) { + VIR_INFO("Target '%s' is already mounted", def->target.path); + return 0; + } + + if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) + return -1; + + cmd = virStorageBackendFileSystemMountCmd(def, src); if (virCommandRun(cmd, NULL) < 0) goto cleanup; -- 2.17.2
Move virStorageBackendFileSystemMountCmd to storage_util so that it can be used by the test harness. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 52 -------------------------------- src/storage/storage_util.c | 52 ++++++++++++++++++++++++++++++++ src/storage/storage_util.h | 4 +++ 3 files changed, 56 insertions(+), 52 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4bf411b330..b341ba84fa 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -331,58 +331,6 @@ virStorageBackendFileSystemIsMounted(virStoragePoolObjPtr pool) } -static virCommandPtr -virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, - const char *src) -{ - /* 'mount -t auto' doesn't seem to auto determine nfs (or cifs), - * while plain 'mount' does. We have to craft separate argvs to - * accommodate this */ - bool netauto = (def->type == VIR_STORAGE_POOL_NETFS && - def->source.format == VIR_STORAGE_POOL_NETFS_AUTO); - bool glusterfs = (def->type == VIR_STORAGE_POOL_NETFS && - def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS); - bool cifsfs = (def->type == VIR_STORAGE_POOL_NETFS && - def->source.format == VIR_STORAGE_POOL_NETFS_CIFS); - virCommandPtr cmd = NULL; - - if (netauto) - cmd = virCommandNewArgList(MOUNT, - src, - def->target.path, - NULL); - else if (glusterfs) - cmd = virCommandNewArgList(MOUNT, - "-t", - virStoragePoolFormatFileSystemNetTypeToString(def->source.format), - src, - "-o", - "direct-io-mode=1", - def->target.path, - NULL); - else if (cifsfs) - cmd = virCommandNewArgList(MOUNT, - "-t", - virStoragePoolFormatFileSystemNetTypeToString(def->source.format), - src, - def->target.path, - "-o", - "guest", - NULL); - else - cmd = virCommandNewArgList(MOUNT, - "-t", - (def->type == VIR_STORAGE_POOL_FS ? - virStoragePoolFormatFileSystemTypeToString(def->source.format) : - virStoragePoolFormatFileSystemNetTypeToString(def->source.format)), - src, - def->target.path, - NULL); - - return cmd; -} - - /** * @pool storage pool to mount * diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 318a556656..180d7b1fa3 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4226,3 +4226,55 @@ virStorageBackendZeroPartitionTable(const char *path, return storageBackendVolWipeLocalFile(path, VIR_STORAGE_VOL_WIPE_ALG_ZERO, size, true); } + + +virCommandPtr +virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, + const char *src) +{ + /* 'mount -t auto' doesn't seem to auto determine nfs (or cifs), + * while plain 'mount' does. We have to craft separate argvs to + * accommodate this */ + bool netauto = (def->type == VIR_STORAGE_POOL_NETFS && + def->source.format == VIR_STORAGE_POOL_NETFS_AUTO); + bool glusterfs = (def->type == VIR_STORAGE_POOL_NETFS && + def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS); + bool cifsfs = (def->type == VIR_STORAGE_POOL_NETFS && + def->source.format == VIR_STORAGE_POOL_NETFS_CIFS); + virCommandPtr cmd = NULL; + + if (netauto) + cmd = virCommandNewArgList(MOUNT, + src, + def->target.path, + NULL); + else if (glusterfs) + cmd = virCommandNewArgList(MOUNT, + "-t", + virStoragePoolFormatFileSystemNetTypeToString(def->source.format), + src, + "-o", + "direct-io-mode=1", + def->target.path, + NULL); + else if (cifsfs) + cmd = virCommandNewArgList(MOUNT, + "-t", + virStoragePoolFormatFileSystemNetTypeToString(def->source.format), + src, + def->target.path, + "-o", + "guest", + NULL); + else + cmd = virCommandNewArgList(MOUNT, + "-t", + (def->type == VIR_STORAGE_POOL_FS ? + virStoragePoolFormatFileSystemTypeToString(def->source.format) : + virStoragePoolFormatFileSystemNetTypeToString(def->source.format)), + src, + def->target.path, + NULL); + + return cmd; +} diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index 58b991c772..5b0baf56c4 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -177,4 +177,8 @@ int virStorageBackendZeroPartitionTable(const char *path, unsigned long long size); +virCommandPtr +virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, + const char *src); + #endif /* __VIR_STORAGE_UTIL_H__ */ -- 2.17.2
Move into storage_util for reuse by test harness Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 33 -------------------------------- src/storage/storage_util.c | 33 ++++++++++++++++++++++++++++++++ src/storage/storage_util.h | 3 +++ 3 files changed, 36 insertions(+), 33 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b341ba84fa..c5e75627b5 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -245,39 +245,6 @@ virStorageBackendFileSystemIsValid(virStoragePoolObjPtr pool) } -/** - * virStorageBackendFileSystemGetPoolSource - * @pool: storage pool object pointer - * - * Allocate/return a string representing the FS storage pool source. - * It is up to the caller to VIR_FREE the allocated string - */ -static char * -virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool) -{ - virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - char *src = NULL; - - if (def->type == VIR_STORAGE_POOL_NETFS) { - if (def->source.format == VIR_STORAGE_POOL_NETFS_CIFS) { - if (virAsprintf(&src, "//%s/%s", - def->source.hosts[0].name, - def->source.dir) < 0) - return NULL; - } else { - if (virAsprintf(&src, "%s:%s", - def->source.hosts[0].name, - def->source.dir) < 0) - return NULL; - } - } else { - if (VIR_STRDUP(src, def->source.devices[0].path) < 0) - return NULL; - } - return src; -} - - /** * @pool storage pool to check for status * diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 180d7b1fa3..c9f6096687 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4228,6 +4228,39 @@ virStorageBackendZeroPartitionTable(const char *path, } +/** + * virStorageBackendFileSystemGetPoolSource + * @pool: storage pool object pointer + * + * Allocate/return a string representing the FS storage pool source. + * It is up to the caller to VIR_FREE the allocated string + */ +char * +virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool) +{ + virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); + char *src = NULL; + + if (def->type == VIR_STORAGE_POOL_NETFS) { + if (def->source.format == VIR_STORAGE_POOL_NETFS_CIFS) { + if (virAsprintf(&src, "//%s/%s", + def->source.hosts[0].name, + def->source.dir) < 0) + return NULL; + } else { + if (virAsprintf(&src, "%s:%s", + def->source.hosts[0].name, + def->source.dir) < 0) + return NULL; + } + } else { + if (VIR_STRDUP(src, def->source.devices[0].path) < 0) + return NULL; + } + return src; +} + + virCommandPtr virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, const char *src) diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index 5b0baf56c4..28b3e0b9c9 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -177,6 +177,9 @@ int virStorageBackendZeroPartitionTable(const char *path, unsigned long long size); +char * +virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool); + virCommandPtr virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, const char *src); -- 2.17.2
Similar to qemuxml2argv and storagevolxml2argv, let's create some tests to ensure that the XML generates a consistent command line. Using the same list of pools as storagepoolxml2xmltest, start with the file system tests (fs, netfs, netfs-cifs, netfs-gluster). Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/Makefile.am | 12 ++ tests/storagepoolxml2argvdata/pool-fs.argv | 1 + .../pool-netfs-cifs.argv | 1 + .../pool-netfs-gluster.argv | 1 + tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + tests/storagepoolxml2argvtest.c | 171 ++++++++++++++++++ 6 files changed, 187 insertions(+) create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv create mode 100644 tests/storagepoolxml2argvtest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index d7ec7e3a6f..bec0930c11 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -140,6 +140,7 @@ EXTRA_DIST = \ storagepoolschemadata \ storagepoolxml2xmlin \ storagepoolxml2xmlout \ + storagepoolxml2argvdata \ storagevolschemadata \ storagevolxml2argvdata \ storagevolxml2xmlin \ @@ -363,6 +364,7 @@ endif WITH_NWFILTER if WITH_STORAGE test_programs += storagevolxml2argvtest +test_programs += storagepoolxml2argvtest test_programs += virstorageutiltest endif WITH_STORAGE @@ -901,6 +903,16 @@ storagevolxml2argvtest_LDADD = \ ../src/libvirt_util.la \ $(LDADDS) +storagepoolxml2argvtest_SOURCES = \ + storagepoolxml2argvtest.c \ + testutils.c testutils.h +storagepoolxml2argvtest_LDADD = \ + $(LIBXML_LIBS) \ + ../src/libvirt_driver_storage_impl.la \ + ../src/libvirt_conf.la \ + ../src/libvirt_util.la \ + $(LDADDS) + else ! WITH_STORAGE EXTRA_DIST += storagevolxml2argvtest.c EXTRA_DIST += virstorageutiltest.c diff --git a/tests/storagepoolxml2argvdata/pool-fs.argv b/tests/storagepoolxml2argvdata/pool-fs.argv new file mode 100644 index 0000000000..4a94148114 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-fs.argv @@ -0,0 +1 @@ +/usr/bin/mount -t ext3 /dev/sda6 /mnt diff --git a/tests/storagepoolxml2argvdata/pool-netfs-cifs.argv b/tests/storagepoolxml2argvdata/pool-netfs-cifs.argv new file mode 100644 index 0000000000..7a06f86f66 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-netfs-cifs.argv @@ -0,0 +1 @@ +/usr/bin/mount -t cifs //example.com/samba_share /mnt/cifs -o guest diff --git a/tests/storagepoolxml2argvdata/pool-netfs-gluster.argv b/tests/storagepoolxml2argvdata/pool-netfs-gluster.argv new file mode 100644 index 0000000000..90e94f6f13 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-netfs-gluster.argv @@ -0,0 +1 @@ +/usr/bin/mount -t glusterfs example.com:/volume -o direct-io-mode=1 /mnt/gluster diff --git a/tests/storagepoolxml2argvdata/pool-netfs.argv b/tests/storagepoolxml2argvdata/pool-netfs.argv new file mode 100644 index 0000000000..f890a03f7e --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-netfs.argv @@ -0,0 +1 @@ +/usr/bin/mount -t nfs localhost:/var/lib/libvirt/images /mnt diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c new file mode 100644 index 0000000000..74f8561379 --- /dev/null +++ b/tests/storagepoolxml2argvtest.c @@ -0,0 +1,171 @@ +#include <config.h> + +#include "internal.h" +#include "testutils.h" +#include "datatypes.h" +#include "storage/storage_util.h" +#include "testutilsqemu.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + + +static int +testCompareXMLToArgvFiles(bool shouldFail, + const char *poolxml, + const char *cmdline) +{ + VIR_AUTOFREE(char *) actualCmdline = NULL; + VIR_AUTOFREE(char *) src = NULL; + int ret = -1; + virCommandPtr cmd = NULL; + virStoragePoolDefPtr def = NULL; + virStoragePoolObjPtr pool = NULL; + + if (!(def = virStoragePoolDefParseFile(poolxml))) + goto cleanup; + + switch ((virStoragePoolType)def->type) { + case VIR_STORAGE_POOL_FS: + case VIR_STORAGE_POOL_NETFS: + if (!(pool = virStoragePoolObjNew())) { + VIR_TEST_DEBUG("pool type %d alloc pool obj fails\n", def->type); + virStoragePoolDefFree(def); + goto cleanup; + } + virStoragePoolObjSetDef(pool, def); + + if (!(src = virStorageBackendFileSystemGetPoolSource(pool))) { + VIR_TEST_DEBUG("pool type %d has no pool source\n", def->type); + goto cleanup; + } + + cmd = virStorageBackendFileSystemMountCmd(def, src); + break; + + case VIR_STORAGE_POOL_DIR: + case VIR_STORAGE_POOL_LOGICAL: + case VIR_STORAGE_POOL_DISK: + case VIR_STORAGE_POOL_ISCSI: + case VIR_STORAGE_POOL_ISCSI_DIRECT: + case VIR_STORAGE_POOL_SCSI: + case VIR_STORAGE_POOL_MPATH: + case VIR_STORAGE_POOL_RBD: + case VIR_STORAGE_POOL_SHEEPDOG: + case VIR_STORAGE_POOL_GLUSTER: + case VIR_STORAGE_POOL_ZFS: + case VIR_STORAGE_POOL_VSTORAGE: + case VIR_STORAGE_POOL_LAST: + default: + VIR_TEST_DEBUG("pool type %d has no xml2argv test\n", def->type); + goto cleanup; + }; + + if (!(actualCmdline = virCommandToString(cmd))) { + VIR_TEST_DEBUG("pool type %d failed to get commandline\n", def->type); + goto cleanup; + } + + if (virTestCompareToFile(actualCmdline, cmdline) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virCommandFree(cmd); + VIR_FREE(actualCmdline); + virStoragePoolObjEndAPI(&pool); + if (shouldFail) { + virResetLastError(); + ret = 0; + } + return ret; +} + +struct testInfo { + bool shouldFail; + const char *pool; +}; + +static int +testCompareXMLToArgvHelper(const void *data) +{ + int result = -1; + const struct testInfo *info = data; + char *poolxml = NULL; + char *cmdline = NULL; + + if (virAsprintf(&poolxml, "%s/storagepoolxml2xmlin/%s.xml", + abs_srcdir, info->pool) < 0) + goto cleanup; + + if (virAsprintf(&cmdline, "%s/storagepoolxml2argvdata/%s.argv", + abs_srcdir, info->pool) < 0 && !info->shouldFail) + goto cleanup; + + result = testCompareXMLToArgvFiles(info->shouldFail, poolxml, cmdline); + + cleanup: + VIR_FREE(poolxml); + VIR_FREE(cmdline); + + return result; +} + + +static int +mymain(void) +{ + int ret = 0; + +#define DO_TEST_FULL(shouldFail, pool) \ + do { \ + struct testInfo info = { shouldFail, pool }; \ + if (virTestRun("Storage Pool XML-2-argv " pool, \ + testCompareXMLToArgvHelper, &info) < 0) \ + ret = -1; \ + } \ + while (0); + +#define DO_TEST(pool, ...) \ + DO_TEST_FULL(false, pool) + +#define DO_TEST_FAIL(pool, ...) \ + DO_TEST_FULL(true, pool) + + DO_TEST_FAIL("pool-dir"); + DO_TEST_FAIL("pool-dir-naming"); + DO_TEST("pool-fs"); + DO_TEST_FAIL("pool-logical"); + DO_TEST_FAIL("pool-logical-nopath"); + DO_TEST_FAIL("pool-logical-create"); + DO_TEST_FAIL("pool-logical-noname"); + DO_TEST_FAIL("pool-disk"); + DO_TEST_FAIL("pool-disk-device-nopartsep"); + DO_TEST_FAIL("pool-iscsi"); + DO_TEST_FAIL("pool-iscsi-auth"); + DO_TEST("pool-netfs"); + DO_TEST("pool-netfs-gluster"); + DO_TEST("pool-netfs-cifs"); + DO_TEST_FAIL("pool-scsi"); + DO_TEST_FAIL("pool-scsi-type-scsi-host"); + DO_TEST_FAIL("pool-scsi-type-fc-host"); + DO_TEST_FAIL("pool-scsi-type-fc-host-managed"); + DO_TEST_FAIL("pool-mpath"); + DO_TEST_FAIL("pool-iscsi-multiiqn"); + DO_TEST_FAIL("pool-iscsi-vendor-product"); + DO_TEST_FAIL("pool-sheepdog"); + DO_TEST_FAIL("pool-gluster"); + DO_TEST_FAIL("pool-gluster-sub"); + DO_TEST_FAIL("pool-scsi-type-scsi-host-stable"); + DO_TEST_FAIL("pool-zfs"); + DO_TEST_FAIL("pool-zfs-sourcedev"); + DO_TEST_FAIL("pool-rbd"); + DO_TEST_FAIL("pool-vstorage"); + DO_TEST_FAIL("pool-iscsi-direct-auth"); + DO_TEST_FAIL("pool-iscsi-direct"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) -- 2.17.2
On 12/4/18 5:47 PM, John Ferlan wrote:
Similar to qemuxml2argv and storagevolxml2argv, let's create some tests to ensure that the XML generates a consistent command line.
Using the same list of pools as storagepoolxml2xmltest, start with the file system tests (fs, netfs, netfs-cifs, netfs-gluster).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/Makefile.am | 12 ++ tests/storagepoolxml2argvdata/pool-fs.argv | 1 + .../pool-netfs-cifs.argv | 1 + .../pool-netfs-gluster.argv | 1 + tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + tests/storagepoolxml2argvtest.c | 171 ++++++++++++++++++ 6 files changed, 187 insertions(+) create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv create mode 100644 tests/storagepoolxml2argvtest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am index d7ec7e3a6f..bec0930c11 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -140,6 +140,7 @@ EXTRA_DIST = \ storagepoolschemadata \ storagepoolxml2xmlin \ storagepoolxml2xmlout \ + storagepoolxml2argvdata \ storagevolschemadata \ storagevolxml2argvdata \ storagevolxml2xmlin \ @@ -363,6 +364,7 @@ endif WITH_NWFILTER
if WITH_STORAGE test_programs += storagevolxml2argvtest +test_programs += storagepoolxml2argvtest test_programs += virstorageutiltest endif WITH_STORAGE
@@ -901,6 +903,16 @@ storagevolxml2argvtest_LDADD = \ ../src/libvirt_util.la \ $(LDADDS)
+storagepoolxml2argvtest_SOURCES = \ + storagepoolxml2argvtest.c \ + testutils.c testutils.h +storagepoolxml2argvtest_LDADD = \ + $(LIBXML_LIBS) \ + ../src/libvirt_driver_storage_impl.la \ + ../src/libvirt_conf.la \ + ../src/libvirt_util.la \ + $(LDADDS) + else ! WITH_STORAGE EXTRA_DIST += storagevolxml2argvtest.c EXTRA_DIST += virstorageutiltest.c diff --git a/tests/storagepoolxml2argvdata/pool-fs.argv b/tests/storagepoolxml2argvdata/pool-fs.argv new file mode 100644 index 0000000000..4a94148114 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-fs.argv @@ -0,0 +1 @@ +/usr/bin/mount -t ext3 /dev/sda6 /mnt
Problem is, on my system (Gentoo), it is /usr/mount so this is failing for me. I wonder if we should have a placeholder here, say "MOUNT" and then replace it with actual location of 'mount' (taken from config.h) at runtime. Michal
On 12/12/18 9:04 AM, Michal Privoznik wrote:
On 12/4/18 5:47 PM, John Ferlan wrote:
Similar to qemuxml2argv and storagevolxml2argv, let's create some tests to ensure that the XML generates a consistent command line.
Using the same list of pools as storagepoolxml2xmltest, start with the file system tests (fs, netfs, netfs-cifs, netfs-gluster).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/Makefile.am | 12 ++ tests/storagepoolxml2argvdata/pool-fs.argv | 1 + .../pool-netfs-cifs.argv | 1 + .../pool-netfs-gluster.argv | 1 + tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + tests/storagepoolxml2argvtest.c | 171 ++++++++++++++++++ 6 files changed, 187 insertions(+) create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv create mode 100644 tests/storagepoolxml2argvtest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am index d7ec7e3a6f..bec0930c11 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -140,6 +140,7 @@ EXTRA_DIST = \ storagepoolschemadata \ storagepoolxml2xmlin \ storagepoolxml2xmlout \ + storagepoolxml2argvdata \ storagevolschemadata \ storagevolxml2argvdata \ storagevolxml2xmlin \ @@ -363,6 +364,7 @@ endif WITH_NWFILTER
if WITH_STORAGE test_programs += storagevolxml2argvtest +test_programs += storagepoolxml2argvtest test_programs += virstorageutiltest endif WITH_STORAGE
@@ -901,6 +903,16 @@ storagevolxml2argvtest_LDADD = \ ../src/libvirt_util.la \ $(LDADDS)
+storagepoolxml2argvtest_SOURCES = \ + storagepoolxml2argvtest.c \ + testutils.c testutils.h +storagepoolxml2argvtest_LDADD = \ + $(LIBXML_LIBS) \ + ../src/libvirt_driver_storage_impl.la \ + ../src/libvirt_conf.la \ + ../src/libvirt_util.la \ + $(LDADDS) + else ! WITH_STORAGE EXTRA_DIST += storagevolxml2argvtest.c EXTRA_DIST += virstorageutiltest.c diff --git a/tests/storagepoolxml2argvdata/pool-fs.argv b/tests/storagepoolxml2argvdata/pool-fs.argv new file mode 100644 index 0000000000..4a94148114 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-fs.argv @@ -0,0 +1 @@ +/usr/bin/mount -t ext3 /dev/sda6 /mnt
Problem is, on my system (Gentoo), it is /usr/mount so this is failing for me.
I wonder if we should have a placeholder here, say "MOUNT" and then replace it with actual location of 'mount' (taken from config.h) at runtime.
Michal
If I add a call to virTestClearCommandPath(actualCmdline); just prior to the if (virTestCompareToFile(actualCmdline, cmdline) < 0) *and* I modify each of the .argv files to remove the leading path, then things work just fine. Would you like to see or is the explanation good enough? IDC either way. The .argv files would be affected/changed here, patch5 (netfs-auto), and patch8 (logical). Tks - John
On 12/12/18 10:33 PM, John Ferlan wrote:
On 12/12/18 9:04 AM, Michal Privoznik wrote:
On 12/4/18 5:47 PM, John Ferlan wrote:
Similar to qemuxml2argv and storagevolxml2argv, let's create some tests to ensure that the XML generates a consistent command line.
Using the same list of pools as storagepoolxml2xmltest, start with the file system tests (fs, netfs, netfs-cifs, netfs-gluster).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/Makefile.am | 12 ++ tests/storagepoolxml2argvdata/pool-fs.argv | 1 + .../pool-netfs-cifs.argv | 1 + .../pool-netfs-gluster.argv | 1 + tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + tests/storagepoolxml2argvtest.c | 171 ++++++++++++++++++ 6 files changed, 187 insertions(+) create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv create mode 100644 tests/storagepoolxml2argvtest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am index d7ec7e3a6f..bec0930c11 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -140,6 +140,7 @@ EXTRA_DIST = \ storagepoolschemadata \ storagepoolxml2xmlin \ storagepoolxml2xmlout \ + storagepoolxml2argvdata \ storagevolschemadata \ storagevolxml2argvdata \ storagevolxml2xmlin \ @@ -363,6 +364,7 @@ endif WITH_NWFILTER
if WITH_STORAGE test_programs += storagevolxml2argvtest +test_programs += storagepoolxml2argvtest test_programs += virstorageutiltest endif WITH_STORAGE
@@ -901,6 +903,16 @@ storagevolxml2argvtest_LDADD = \ ../src/libvirt_util.la \ $(LDADDS)
+storagepoolxml2argvtest_SOURCES = \ + storagepoolxml2argvtest.c \ + testutils.c testutils.h +storagepoolxml2argvtest_LDADD = \ + $(LIBXML_LIBS) \ + ../src/libvirt_driver_storage_impl.la \ + ../src/libvirt_conf.la \ + ../src/libvirt_util.la \ + $(LDADDS) + else ! WITH_STORAGE EXTRA_DIST += storagevolxml2argvtest.c EXTRA_DIST += virstorageutiltest.c diff --git a/tests/storagepoolxml2argvdata/pool-fs.argv b/tests/storagepoolxml2argvdata/pool-fs.argv new file mode 100644 index 0000000000..4a94148114 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-fs.argv @@ -0,0 +1 @@ +/usr/bin/mount -t ext3 /dev/sda6 /mnt
Problem is, on my system (Gentoo), it is /usr/mount so this is failing for me.
I wonder if we should have a placeholder here, say "MOUNT" and then replace it with actual location of 'mount' (taken from config.h) at runtime.
Michal
If I add a call to virTestClearCommandPath(actualCmdline); just prior to the if (virTestCompareToFile(actualCmdline, cmdline) < 0) *and* I modify each of the .argv files to remove the leading path, then things work just fine.
Ah, had no idea we have such nifty function.
Would you like to see or is the explanation good enough? IDC either way.
I believe you can place the function call at the right place and regenerate the test outputs. ACK series then. Michal
Cover the case where @netauto would be used to create the command line in virStorageBackendFileSystemMountCmd. Essentially when the pool type is "netfs", but the "source.format" is empty, create the command line properly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- .../pool-netfs-auto.argv | 1 + tests/storagepoolxml2argvtest.c | 1 + .../storagepoolxml2xmlin/pool-netfs-auto.xml | 19 ++++++++++++++++++ .../storagepoolxml2xmlout/pool-netfs-auto.xml | 20 +++++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 5 files changed, 42 insertions(+) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-auto.argv create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-auto.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-auto.xml diff --git a/tests/storagepoolxml2argvdata/pool-netfs-auto.argv b/tests/storagepoolxml2argvdata/pool-netfs-auto.argv new file mode 100644 index 0000000000..217fcc786b --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-netfs-auto.argv @@ -0,0 +1 @@ +/usr/bin/mount localhost:/var/lib/libvirt/images /mnt diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index 74f8561379..42044e3518 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -145,6 +145,7 @@ mymain(void) DO_TEST_FAIL("pool-iscsi"); DO_TEST_FAIL("pool-iscsi-auth"); DO_TEST("pool-netfs"); + DO_TEST("pool-netfs-auto"); DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); DO_TEST_FAIL("pool-scsi"); diff --git a/tests/storagepoolxml2xmlin/pool-netfs-auto.xml b/tests/storagepoolxml2xmlin/pool-netfs-auto.xml new file mode 100644 index 0000000000..d7f7ce8168 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-netfs-auto.xml @@ -0,0 +1,19 @@ +<pool type='netfs'> + <name>nfsimages</name> + <uuid>7641d5a8-af11-f730-a34e-0a7dfcede71f</uuid> + <capacity>0</capacity> + <allocation>0</allocation> + <available>0</available> + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + </source> + <target> + <path>/mnt</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-netfs-auto.xml b/tests/storagepoolxml2xmlout/pool-netfs-auto.xml new file mode 100644 index 0000000000..a180ca521c --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-netfs-auto.xml @@ -0,0 +1,20 @@ +<pool type='netfs'> + <name>nfsimages</name> + <uuid>7641d5a8-af11-f730-a34e-0a7dfcede71f</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + <format type='auto'/> + </source> + <target> + <path>/mnt</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 8230dc8ddc..707d09f5c2 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -82,6 +82,7 @@ mymain(void) DO_TEST("pool-iscsi"); DO_TEST("pool-iscsi-auth"); DO_TEST("pool-netfs"); + DO_TEST("pool-netfs-auto"); DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); DO_TEST("pool-scsi"); -- 2.17.2
Let's create helpers for each style of command line created. This primarily is easier on the eyes rather than the large multi line if-then-else-else clause used, but may also be useful if in the future any particular pool needs to add to the command line based on pool xml format. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 84 +++++++++++++++++++++++++------------- 1 file changed, 55 insertions(+), 29 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index c9f6096687..789f270f2a 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4261,6 +4261,56 @@ virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool) } +static void +virStorageBackendFileSystemMountNFSArgs(virCommandPtr cmd, + const char *src, + virStoragePoolDefPtr def) +{ + virCommandAddArgList(cmd, src, def->target.path, NULL); +} + + +static void +virStorageBackendFileSystemMountGlusterArgs(virCommandPtr cmd, + const char *src, + virStoragePoolDefPtr def) +{ + const char *fmt; + + fmt = virStoragePoolFormatFileSystemNetTypeToString(def->source.format); + virCommandAddArgList(cmd, "-t", fmt, src, "-o", "direct-io-mode=1", + def->target.path, NULL); +} + + +static void +virStorageBackendFileSystemMountCIFSArgs(virCommandPtr cmd, + const char *src, + virStoragePoolDefPtr def) +{ + const char *fmt; + + fmt = virStoragePoolFormatFileSystemNetTypeToString(def->source.format); + virCommandAddArgList(cmd, "-t", fmt, src, def->target.path, + "-o", "guest", NULL); +} + + +static void +virStorageBackendFileSystemMountDefaultArgs(virCommandPtr cmd, + const char *src, + virStoragePoolDefPtr def) +{ + const char *fmt; + + if (def->type == VIR_STORAGE_POOL_FS) + fmt = virStoragePoolFormatFileSystemTypeToString(def->source.format); + else + fmt = virStoragePoolFormatFileSystemNetTypeToString(def->source.format); + virCommandAddArgList(cmd, "-t", fmt, src, def->target.path, NULL); +} + + virCommandPtr virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, const char *src) @@ -4276,38 +4326,14 @@ virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, def->source.format == VIR_STORAGE_POOL_NETFS_CIFS); virCommandPtr cmd = NULL; + cmd = virCommandNew(MOUNT); if (netauto) - cmd = virCommandNewArgList(MOUNT, - src, - def->target.path, - NULL); + virStorageBackendFileSystemMountNFSArgs(cmd, src, def); else if (glusterfs) - cmd = virCommandNewArgList(MOUNT, - "-t", - virStoragePoolFormatFileSystemNetTypeToString(def->source.format), - src, - "-o", - "direct-io-mode=1", - def->target.path, - NULL); + virStorageBackendFileSystemMountGlusterArgs(cmd, src, def); else if (cifsfs) - cmd = virCommandNewArgList(MOUNT, - "-t", - virStoragePoolFormatFileSystemNetTypeToString(def->source.format), - src, - def->target.path, - "-o", - "guest", - NULL); + virStorageBackendFileSystemMountCIFSArgs(cmd, src, def); else - cmd = virCommandNewArgList(MOUNT, - "-t", - (def->type == VIR_STORAGE_POOL_FS ? - virStoragePoolFormatFileSystemTypeToString(def->source.format) : - virStoragePoolFormatFileSystemNetTypeToString(def->source.format)), - src, - def->target.path, - NULL); - + virStorageBackendFileSystemMountDefaultArgs(cmd, src, def); return cmd; } -- 2.17.2
It's only pass as 0 or 1 and used as a bool, let's just use a bool Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index cad88756cb..44cff61af7 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -48,7 +48,7 @@ VIR_LOG_INIT("storage.storage_backend_logical"); static int virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, - int on) + bool on) { int ret; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); @@ -731,7 +731,7 @@ virStorageBackendLogicalStartPool(virStoragePoolObjPtr pool) * that the pool's source devices match the pvs output. */ if (!virStorageBackendLogicalMatchPoolSource(pool) || - virStorageBackendLogicalSetActive(pool, 1) < 0) + virStorageBackendLogicalSetActive(pool, true) < 0) return -1; return 0; @@ -858,7 +858,7 @@ virStorageBackendLogicalRefreshPool(virStoragePoolObjPtr pool) static int virStorageBackendLogicalStopPool(virStoragePoolObjPtr pool) { - if (virStorageBackendLogicalSetActive(pool, 0) < 0) + if (virStorageBackendLogicalSetActive(pool, false) < 0) return -1; return 0; -- 2.17.2
Add the logical storage pool startup validation (xml2argv) tests. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 6 +----- src/storage/storage_util.c | 11 +++++++++++ src/storage/storage_util.h | 4 ++++ .../pool-logical-create.argv | 1 + .../pool-logical-noname.argv | 1 + .../pool-logical-nopath.argv | 1 + tests/storagepoolxml2argvdata/pool-logical.argv | 1 + tests/storagepoolxml2argvtest.c | 13 ++++++++----- 8 files changed, 28 insertions(+), 10 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-logical-create.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-noname.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-nopath.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical.argv diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 44cff61af7..12fff651e8 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -52,11 +52,7 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, { int ret; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = - virCommandNewArgList(VGCHANGE, - on ? "-aly" : "-aln", - def->source.name, - NULL); + virCommandPtr cmd = virStorageBackendLogicalChangeCmd(def, on); ret = virCommandRun(cmd, NULL); virCommandFree(cmd); diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 789f270f2a..01f3c93008 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4337,3 +4337,14 @@ virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, virStorageBackendFileSystemMountDefaultArgs(cmd, src, def); return cmd; } + + +virCommandPtr +virStorageBackendLogicalChangeCmd(virStoragePoolDefPtr def, + bool on) +{ + return virCommandNewArgList(VGCHANGE, + on ? "-aly" : "-aln", + def->source.name, + NULL); +} diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index 28b3e0b9c9..a2ef2ac07d 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -184,4 +184,8 @@ virCommandPtr virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, const char *src); +virCommandPtr +virStorageBackendLogicalChangeCmd(virStoragePoolDefPtr def, + bool on); + #endif /* __VIR_STORAGE_UTIL_H__ */ diff --git a/tests/storagepoolxml2argvdata/pool-logical-create.argv b/tests/storagepoolxml2argvdata/pool-logical-create.argv new file mode 100644 index 0000000000..203da86e48 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-logical-create.argv @@ -0,0 +1 @@ +/usr/sbin/vgchange -aly HostVG diff --git a/tests/storagepoolxml2argvdata/pool-logical-noname.argv b/tests/storagepoolxml2argvdata/pool-logical-noname.argv new file mode 100644 index 0000000000..0813467120 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-logical-noname.argv @@ -0,0 +1 @@ +/usr/sbin/vgchange -aly zily diff --git a/tests/storagepoolxml2argvdata/pool-logical-nopath.argv b/tests/storagepoolxml2argvdata/pool-logical-nopath.argv new file mode 100644 index 0000000000..203da86e48 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-logical-nopath.argv @@ -0,0 +1 @@ +/usr/sbin/vgchange -aly HostVG diff --git a/tests/storagepoolxml2argvdata/pool-logical.argv b/tests/storagepoolxml2argvdata/pool-logical.argv new file mode 100644 index 0000000000..203da86e48 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-logical.argv @@ -0,0 +1 @@ +/usr/sbin/vgchange -aly HostVG diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index 42044e3518..26b12c8fc3 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -43,8 +43,11 @@ testCompareXMLToArgvFiles(bool shouldFail, cmd = virStorageBackendFileSystemMountCmd(def, src); break; - case VIR_STORAGE_POOL_DIR: case VIR_STORAGE_POOL_LOGICAL: + cmd = virStorageBackendLogicalChangeCmd(def, true); + break; + + case VIR_STORAGE_POOL_DIR: case VIR_STORAGE_POOL_DISK: case VIR_STORAGE_POOL_ISCSI: case VIR_STORAGE_POOL_ISCSI_DIRECT: @@ -136,10 +139,10 @@ mymain(void) DO_TEST_FAIL("pool-dir"); DO_TEST_FAIL("pool-dir-naming"); DO_TEST("pool-fs"); - DO_TEST_FAIL("pool-logical"); - DO_TEST_FAIL("pool-logical-nopath"); - DO_TEST_FAIL("pool-logical-create"); - DO_TEST_FAIL("pool-logical-noname"); + DO_TEST("pool-logical"); + DO_TEST("pool-logical-nopath"); + DO_TEST("pool-logical-create"); + DO_TEST("pool-logical-noname"); DO_TEST_FAIL("pool-disk"); DO_TEST_FAIL("pool-disk-device-nopartsep"); DO_TEST_FAIL("pool-iscsi"); -- 2.17.2
On 12/4/18 5:47 PM, John Ferlan wrote:
Add the logical storage pool startup validation (xml2argv) tests.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 6 +----- src/storage/storage_util.c | 11 +++++++++++ src/storage/storage_util.h | 4 ++++ .../pool-logical-create.argv | 1 + .../pool-logical-noname.argv | 1 + .../pool-logical-nopath.argv | 1 + tests/storagepoolxml2argvdata/pool-logical.argv | 1 + tests/storagepoolxml2argvtest.c | 13 ++++++++----- 8 files changed, 28 insertions(+), 10 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-logical-create.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-noname.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-nopath.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical.argv
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 44cff61af7..12fff651e8 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -52,11 +52,7 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, { int ret; virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - virCommandPtr cmd = - virCommandNewArgList(VGCHANGE, - on ? "-aly" : "-aln", - def->source.name, - NULL); + virCommandPtr cmd = virStorageBackendLogicalChangeCmd(def, on);
ret = virCommandRun(cmd, NULL); virCommandFree(cmd); diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 789f270f2a..01f3c93008 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4337,3 +4337,14 @@ virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, virStorageBackendFileSystemMountDefaultArgs(cmd, src, def); return cmd; } + + +virCommandPtr +virStorageBackendLogicalChangeCmd(virStoragePoolDefPtr def, + bool on) +{ + return virCommandNewArgList(VGCHANGE, + on ? "-aly" : "-aln", + def->source.name, + NULL); +} diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index 28b3e0b9c9..a2ef2ac07d 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -184,4 +184,8 @@ virCommandPtr virStorageBackendFileSystemMountCmd(virStoragePoolDefPtr def, const char *src);
+virCommandPtr +virStorageBackendLogicalChangeCmd(virStoragePoolDefPtr def, + bool on); + #endif /* __VIR_STORAGE_UTIL_H__ */ diff --git a/tests/storagepoolxml2argvdata/pool-logical-create.argv b/tests/storagepoolxml2argvdata/pool-logical-create.argv new file mode 100644 index 0000000000..203da86e48 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-logical-create.argv @@ -0,0 +1 @@ +/usr/sbin/vgchange -aly HostVG
The same point here. On my system it is /sbin/vgchange. Michal
ping? Thanks, John On 12/4/18 11:47 AM, John Ferlan wrote:
Similar to qemuxml2argv and storagevolxml2argv, add storagepoolxml2argvtest in order to check the command line creation for the pool 'Start' commands.
Only applicable for pool types with a "@startPool" function - that is disk, fs, iscsi, logical, scsi, and vstorage and further restricted if the pool doesn't use virCommandPtr type processing.
This initial series only addresses fs and logical so that it's not "too many patches to review".
The iscsi and vstorage pools could have their own tests, with the iscsi ones being more challenging to write.
The disk pool does not have the normal command line processing to start something - rather the command line processing is used to validate that the pool about to be started is of a valid type based on the label seen at startup compared to the pool XML. This processing is not easily mocked.
The scsi (for NPIV) doesn't use the virCommandPtr style interfaces, but at least that processing is tested in other ways using testCreateVport from test_driver.c.
John Ferlan (8): storage: Extract out mount command creation for FS Backend storage: Move FS backend mount creation command helper storage: Move virStorageBackendFileSystemGetPoolSource tests: Introduce tests for storage pool xml to argv checks tests: Add storagepool xml test for netfs-auto storage: Rework virStorageBackendFileSystemMountCmd logical: Fix @on argument type storage: Add tests for logical backend startup
src/storage/storage_backend_fs.c | 77 +------- src/storage/storage_backend_logical.c | 12 +- src/storage/storage_util.c | 122 ++++++++++++ src/storage/storage_util.h | 11 ++ tests/Makefile.am | 12 ++ tests/storagepoolxml2argvdata/pool-fs.argv | 1 + .../pool-logical-create.argv | 1 + .../pool-logical-noname.argv | 1 + .../pool-logical-nopath.argv | 1 + .../storagepoolxml2argvdata/pool-logical.argv | 1 + .../pool-netfs-auto.argv | 1 + .../pool-netfs-cifs.argv | 1 + .../pool-netfs-gluster.argv | 1 + tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + tests/storagepoolxml2argvtest.c | 175 ++++++++++++++++++ .../storagepoolxml2xmlin/pool-netfs-auto.xml | 19 ++ .../storagepoolxml2xmlout/pool-netfs-auto.xml | 20 ++ tests/storagepoolxml2xmltest.c | 1 + 18 files changed, 375 insertions(+), 83 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-create.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-noname.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-nopath.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-auto.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv create mode 100644 tests/storagepoolxml2argvtest.c create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-auto.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-auto.xml
On 12/4/18 5:47 PM, John Ferlan wrote:
Similar to qemuxml2argv and storagevolxml2argv, add storagepoolxml2argvtest in order to check the command line creation for the pool 'Start' commands.
Only applicable for pool types with a "@startPool" function - that is disk, fs, iscsi, logical, scsi, and vstorage and further restricted if the pool doesn't use virCommandPtr type processing.
This initial series only addresses fs and logical so that it's not "too many patches to review".
The iscsi and vstorage pools could have their own tests, with the iscsi ones being more challenging to write.
The disk pool does not have the normal command line processing to start something - rather the command line processing is used to validate that the pool about to be started is of a valid type based on the label seen at startup compared to the pool XML. This processing is not easily mocked.
The scsi (for NPIV) doesn't use the virCommandPtr style interfaces, but at least that processing is tested in other ways using testCreateVport from test_driver.c.
John Ferlan (8): storage: Extract out mount command creation for FS Backend storage: Move FS backend mount creation command helper storage: Move virStorageBackendFileSystemGetPoolSource tests: Introduce tests for storage pool xml to argv checks tests: Add storagepool xml test for netfs-auto storage: Rework virStorageBackendFileSystemMountCmd logical: Fix @on argument type storage: Add tests for logical backend startup
src/storage/storage_backend_fs.c | 77 +------- src/storage/storage_backend_logical.c | 12 +- src/storage/storage_util.c | 122 ++++++++++++ src/storage/storage_util.h | 11 ++ tests/Makefile.am | 12 ++ tests/storagepoolxml2argvdata/pool-fs.argv | 1 + .../pool-logical-create.argv | 1 + .../pool-logical-noname.argv | 1 + .../pool-logical-nopath.argv | 1 + .../storagepoolxml2argvdata/pool-logical.argv | 1 + .../pool-netfs-auto.argv | 1 + .../pool-netfs-cifs.argv | 1 + .../pool-netfs-gluster.argv | 1 + tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + tests/storagepoolxml2argvtest.c | 175 ++++++++++++++++++ .../storagepoolxml2xmlin/pool-netfs-auto.xml | 19 ++ .../storagepoolxml2xmlout/pool-netfs-auto.xml | 20 ++ tests/storagepoolxml2xmltest.c | 1 + 18 files changed, 375 insertions(+), 83 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-create.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-noname.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-nopath.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-auto.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv create mode 100644 tests/storagepoolxml2argvtest.c create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-auto.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-auto.xml
The patches look good from logical POV. However, there is an issue with mount/vgchange location on different systems. Michal
On 12/12/18 9:04 AM, Michal Privoznik wrote:
On 12/4/18 5:47 PM, John Ferlan wrote:
Similar to qemuxml2argv and storagevolxml2argv, add storagepoolxml2argvtest in order to check the command line creation for the pool 'Start' commands.
Only applicable for pool types with a "@startPool" function - that is disk, fs, iscsi, logical, scsi, and vstorage and further restricted if the pool doesn't use virCommandPtr type processing.
This initial series only addresses fs and logical so that it's not "too many patches to review".
The iscsi and vstorage pools could have their own tests, with the iscsi ones being more challenging to write.
The disk pool does not have the normal command line processing to start something - rather the command line processing is used to validate that the pool about to be started is of a valid type based on the label seen at startup compared to the pool XML. This processing is not easily mocked.
The scsi (for NPIV) doesn't use the virCommandPtr style interfaces, but at least that processing is tested in other ways using testCreateVport from test_driver.c.
John Ferlan (8): storage: Extract out mount command creation for FS Backend storage: Move FS backend mount creation command helper storage: Move virStorageBackendFileSystemGetPoolSource tests: Introduce tests for storage pool xml to argv checks tests: Add storagepool xml test for netfs-auto storage: Rework virStorageBackendFileSystemMountCmd logical: Fix @on argument type storage: Add tests for logical backend startup
src/storage/storage_backend_fs.c | 77 +------- src/storage/storage_backend_logical.c | 12 +- src/storage/storage_util.c | 122 ++++++++++++ src/storage/storage_util.h | 11 ++ tests/Makefile.am | 12 ++ tests/storagepoolxml2argvdata/pool-fs.argv | 1 + .../pool-logical-create.argv | 1 + .../pool-logical-noname.argv | 1 + .../pool-logical-nopath.argv | 1 + .../storagepoolxml2argvdata/pool-logical.argv | 1 + .../pool-netfs-auto.argv | .../pool-netfs-cifs.argv | 1 + .../pool-netfs-gluster.argv | 1 + tests/storagepoolxml2argvdata/pool-netfs.argv | 1 + tests/storagepoolxml2argvtest.c | 175 ++++++++++++++++++ .../storagepoolxml2xmlin/pool-netfs-auto.xml | 19 ++ .../storagepoolxml2xmlout/pool-netfs-auto.xml | 20 ++ tests/storagepoolxml2xmltest.c | 1 + 18 files changed, 375 insertions(+), 83 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-fs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-create.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-noname.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical-nopath.argv create mode 100644 tests/storagepoolxml2argvdata/pool-logical.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-auto.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-cifs.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-gluster.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs.argv create mode 100644 tests/storagepoolxml2argvtest.c create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-auto.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-auto.xml
The patches look good from logical POV. However, there is an issue with mount/vgchange location on different systems.
Michal
Hmm... good to know... Maybe I can strip the path to mount/vgchange from the output... That shouldn't be hard to do (hah!). I'll see if I can come up with something... Tks - John
participants (2)
-
John Ferlan -
Michal Privoznik