[libvirt] [PATCH v2 0/3] Adjust netfs CIFS/Samba formatting
https://bugzilla.redhat.com/show_bug.cgi?id=1186969 v1 here: http://www.redhat.com/archives/libvir-list/2015-June/msg00155.html Changes since v1: * Alter v1 to make a common RNG - even though it's ACK'd - I figured I'd still send to make sure there were no other issues... * Add 2/3 to apply comment made in former 2/2 regarding checking for pool->def->type == VIR_STORAGE_POOL_FS to the glusterfs since it has the same issue * Modify 3/3 is old 2/2 and only removed the ternary check. John Ferlan (3): storage: Fix the schema and add tests for cifs pool storage: Adjust command arglist for gluster storage: Generate correct parameters for CIFS docs/formatstorage.html.in | 7 ++++-- docs/schemas/storagepool.rng | 10 ++++---- docs/storage.html.in | 3 ++- src/storage/storage_backend_fs.c | 31 ++++++++++++++++++------- tests/storagepoolxml2xmlin/pool-netfs-cifs.xml | 12 ++++++++++ tests/storagepoolxml2xmlout/pool-netfs-cifs.xml | 15 ++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 64 insertions(+), 15 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-cifs.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-cifs.xml -- 2.1.0
Commit id '887dd362' added support for a netfs pool format type 'cifs' and 'gluster' in order to add rng support for Samba and glusterfs netfs pools. Originally, the CIFS type support was added as part of commit id '61fb6979'. Eventually commit id 'b325be12' fixed the gluster rng definition to match expectations. As it turns out the CIFS rng needed a similar change since the directory path is not an absDirPath, rather just a dirPath will be required. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/schemas/storagepool.rng | 10 ++++++---- tests/storagepoolxml2xmlin/pool-netfs-cifs.xml | 12 ++++++++++++ tests/storagepoolxml2xmlout/pool-netfs-cifs.xml | 15 +++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 4 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-cifs.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-cifs.xml diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index db6ff49..9c52817 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -314,7 +314,7 @@ </element> </define> - <define name='sourceinfonetfsgluster'> + <define name='sourceinfonetrelativepath'> <element name='dir'> <attribute name='path'> <ref name='dirPath'/> @@ -400,7 +400,6 @@ <choice> <value>auto</value> <value>nfs</value> - <value>cifs</value> </choice> </attribute> </element> @@ -488,10 +487,13 @@ <group> <interleave> <ref name='sourceinfohost'/> - <ref name='sourceinfonetfsgluster'/> + <ref name='sourceinfonetrelativepath'/> <element name='format'> <attribute name='type'> - <value>glusterfs</value> + <choice> + <value>cifs</value> + <value>glusterfs</value> + </choice> </attribute> </element> <optional> diff --git a/tests/storagepoolxml2xmlin/pool-netfs-cifs.xml b/tests/storagepoolxml2xmlin/pool-netfs-cifs.xml new file mode 100644 index 0000000..0bc6380 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-netfs-cifs.xml @@ -0,0 +1,12 @@ +<pool type='netfs'> + <source> + <host name='example.com'/> + <format type='cifs'/> + <dir path='samba_share'/> + </source> + <name>netfs-cifs</name> + <uuid>d5609ced-94b1-489e-b218-eff35c30336a</uuid> + <target> + <path>/mnt/cifs</path> + </target> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-netfs-cifs.xml b/tests/storagepoolxml2xmlout/pool-netfs-cifs.xml new file mode 100644 index 0000000..afaa7d0 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-netfs-cifs.xml @@ -0,0 +1,15 @@ +<pool type='netfs'> + <name>netfs-cifs</name> + <uuid>d5609ced-94b1-489e-b218-eff35c30336a</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='example.com'/> + <dir path='samba_share'/> + <format type='cifs'/> + </source> + <target> + <path>/mnt/cifs</path> + </target> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index bec1b8f..b03c4b0 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -84,6 +84,7 @@ mymain(void) DO_TEST("pool-iscsi-auth"); DO_TEST("pool-netfs"); DO_TEST("pool-netfs-gluster"); + DO_TEST("pool-netfs-cifs"); DO_TEST("pool-scsi"); DO_TEST("pool-scsi-type-scsi-host"); DO_TEST("pool-scsi-type-fc-host"); -- 2.1.0
On Mon, Jun 15, 2015 at 17:30:35 -0400, John Ferlan wrote:
Commit id '887dd362' added support for a netfs pool format type 'cifs' and 'gluster' in order to add rng support for Samba and glusterfs netfs pools. Originally, the CIFS type support was added as part of commit id '61fb6979'. Eventually commit id 'b325be12' fixed the gluster rng definition to match expectations.
As it turns out the CIFS rng needed a similar change since the directory path is not an absDirPath, rather just a dirPath will be required.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/schemas/storagepool.rng | 10 ++++++---- tests/storagepoolxml2xmlin/pool-netfs-cifs.xml | 12 ++++++++++++ tests/storagepoolxml2xmlout/pool-netfs-cifs.xml | 15 +++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 4 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-cifs.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-cifs.xml
ACK, Peter
In order for the glusterfs boolean to be set, the pool->def->type must be VIR_STORAGE_POOL_NETFS, thus the check within virCommandNewArgList whether pool->def->type is VIR_STORAGE_POOL_FS will never be true, so remove it Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 8487f08..d2cf470 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -462,9 +462,7 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) else if (glusterfs) cmd = virCommandNewArgList(MOUNT, "-t", - (pool->def->type == VIR_STORAGE_POOL_FS ? - virStoragePoolFormatFileSystemTypeToString(pool->def->source.format) : - virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format)), + virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format), src, "-o", "direct-io-mode=1", -- 2.1.0
On Mon, Jun 15, 2015 at 17:30:36 -0400, John Ferlan wrote:
In order for the glusterfs boolean to be set, the pool->def->type must be VIR_STORAGE_POOL_NETFS, thus the check within virCommandNewArgList whether pool->def->type is VIR_STORAGE_POOL_FS will never be true, so remove it
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
ACK, Peter
https://bugzilla.redhat.com/show_bug.cgi?id=1186969 When generating the path to the dir for a CIFS/Samba driver, the code would generate a source path for the mount using "%s:%s" while the mount.cifs expects to see "//%s/%s". So check for the cifsfs and format the source path appropriately. Additionally, since there is no means to authenticate, the mount needs a "-o guest" on the command line in order to anonymously mount the Samba directory. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 7 +++++-- docs/storage.html.in | 3 ++- src/storage/storage_backend_fs.c | 27 ++++++++++++++++++++++----- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 17558f8..b6f4361 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -124,11 +124,14 @@ <span class="since">Since 0.4.1</span></dd> <dt><code>dir</code></dt> <dd>Provides the source for pools backed by directories (pool - type <code>dir</code>), or optionally to select a subdirectory + types <code>dir</code>, <code>netfs</code>, <code>gluster</code>), + or optionally to select a subdirectory within a pool that resembles a filesystem (pool type <code>gluster</code>). May only occur once. Contains a single attribute <code>path</code> - which is the fully qualified path to the backing directory. + which is the fully qualified path to the backing directory or + for a <code>netfs</code> pool type using <code>format</code> + type "cifs", the path to the Samba share without the leading slash. <span class="since">Since 0.4.1</span></dd> <dt><code>adapter</code></dt> <dd>Provides the source for pools backed by SCSI adapters (pool diff --git a/docs/storage.html.in b/docs/storage.html.in index 92e9ae7..0b467d5 100644 --- a/docs/storage.html.in +++ b/docs/storage.html.in @@ -291,7 +291,8 @@ the <a href="#StorageBackendGluster">gluster</a> pool.) </li> <li> - <code>cifs</code> - use the SMB (samba) or CIFS file system + <code>cifs</code> - use the SMB (samba) or CIFS file system. + The mount will use "-o guest" to mount the directory anonymously. </li> </ul> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index d2cf470..b751687 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -426,6 +426,8 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) pool->def->source.format == VIR_STORAGE_POOL_NETFS_AUTO); bool glusterfs = (pool->def->type == VIR_STORAGE_POOL_NETFS && pool->def->source.format == VIR_STORAGE_POOL_NETFS_GLUSTERFS); + bool cifsfs = (pool->def->type == VIR_STORAGE_POOL_NETFS && + pool->def->source.format == VIR_STORAGE_POOL_NETFS_CIFS); virCommandPtr cmd = NULL; int ret = -1; int rc; @@ -444,11 +446,17 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) } if (pool->def->type == VIR_STORAGE_POOL_NETFS) { - if (virAsprintf(&src, "%s:%s", - pool->def->source.hosts[0].name, - pool->def->source.dir) == -1) - return -1; - + if (pool->def->source.format == VIR_STORAGE_POOL_NETFS_CIFS) { + if (virAsprintf(&src, "//%s/%s", + pool->def->source.hosts[0].name, + pool->def->source.dir) == -1) + return -1; + } else { + if (virAsprintf(&src, "%s:%s", + pool->def->source.hosts[0].name, + pool->def->source.dir) == -1) + return -1; + } } else { if (VIR_STRDUP(src, pool->def->source.devices[0].path) < 0) return -1; @@ -468,6 +476,15 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) "direct-io-mode=1", pool->def->target.path, NULL); + else if (cifsfs) + cmd = virCommandNewArgList(MOUNT, + "-t", + virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format), + src, + pool->def->target.path, + "-o", + "guest", + NULL); else cmd = virCommandNewArgList(MOUNT, "-t", -- 2.1.0
On Mon, Jun 15, 2015 at 17:30:37 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1186969
When generating the path to the dir for a CIFS/Samba driver, the code would generate a source path for the mount using "%s:%s" while the mount.cifs expects to see "//%s/%s". So check for the cifsfs and format the source path appropriately.
Additionally, since there is no means to authenticate, the mount needs a "-o guest" on the command line in order to anonymously mount the Samba directory.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 7 +++++-- docs/storage.html.in | 3 ++- src/storage/storage_backend_fs.c | 27 ++++++++++++++++++++++----- 3 files changed, 29 insertions(+), 8 deletions(-)
ACK, Peter
participants (2)
-
John Ferlan -
Peter Krempa