[libvirt] [PATCH 0/8] Add tests for Storage Pool startup command line generation

[John Ferlan]

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

[John Ferlan]

Move virStorageBackendFileSystemMountCmd to storage_util so that
it can be used by the test harness.

Signed-off-by: John Ferlan <***@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__ */

[John Ferlan]

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 <***@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)

[John Ferlan]

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 <***@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;

[John Ferlan]

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 <***@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");

[John Ferlan]

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 <***@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;
}

[John Ferlan]

It's only pass as 0 or 1 and used as a bool, let's just use a bool

Signed-off-by: John Ferlan <***@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;

[John Ferlan]

Move into storage_util for reuse by test harness

Signed-off-by: John Ferlan <***@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);

[John Ferlan]

Add the logical storage pool startup validation (xml2argv) tests.

Signed-off-by: John Ferlan <***@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");

[John Ferlan]

ping?

Thanks,

John

Post by John Ferlan
Similar to qemuxml2argv and storagevolxml2argv, add storagepoolxml2argvtest
in order to check the command line creation for the pool 'Start' commands.
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.
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
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