[libvirt] [PATCH 0/2] Adjust netfs CIFS/Samba formatting
https://bugzilla.redhat.com/show_bug.cgi?id=1186969 The first patch follows a previous change to fix/adjust the gluster specific rng formatting The second patch resolves the particular issue ensuring to generate the source path with the "//%s/%s" instead of "%s:%s" and with the "-o group" to connect to the Samba serbver. John Ferlan (2): storage: Fix the schema and add tests for cifs pool storage: Generate correct parameters for CIFS docs/formatstorage.html.in | 7 ++++-- docs/schemas/storagepool.rng | 24 +++++++++++++++++++- docs/storage.html.in | 3 ++- src/storage/storage_backend_fs.c | 29 ++++++++++++++++++++----- tests/storagepoolxml2xmlin/pool-netfs-cifs.xml | 12 ++++++++++ tests/storagepoolxml2xmlout/pool-netfs-cifs.xml | 15 +++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 82 insertions(+), 9 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 | 24 +++++++++++++++++++++++- tests/storagepoolxml2xmlin/pool-netfs-cifs.xml | 12 ++++++++++++ tests/storagepoolxml2xmlout/pool-netfs-cifs.xml | 15 +++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 4 files changed, 51 insertions(+), 1 deletion(-) 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..d6bf772 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -314,6 +314,15 @@ </element> </define> + <define name='sourceinfonetfscifs'> + <element name='dir'> + <attribute name='path'> + <ref name='dirPath'/> + </attribute> + <empty/> + </element> + </define> + <define name='sourceinfonetfsgluster'> <element name='dir'> <attribute name='path'> @@ -400,7 +409,6 @@ <choice> <value>auto</value> <value>nfs</value> - <value>cifs</value> </choice> </attribute> </element> @@ -488,6 +496,20 @@ <group> <interleave> <ref name='sourceinfohost'/> + <ref name='sourceinfonetfscifs'/> + <element name='format'> + <attribute name='type'> + <value>cifs</value> + </attribute> + </element> + <optional> + <ref name='sourceinfovendor'/> + </optional> + </interleave> + </group> + <group> + <interleave> + <ref name='sourceinfohost'/> <ref name='sourceinfonetfsgluster'/> <element name='format'> <attribute name='type'> 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 Wed, Jun 03, 2015 at 13:06:57 -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 | 24 +++++++++++++++++++++++- tests/storagepoolxml2xmlin/pool-netfs-cifs.xml | 12 ++++++++++++ tests/storagepoolxml2xmlout/pool-netfs-cifs.xml | 15 +++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 4 files changed, 51 insertions(+), 1 deletion(-) 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..d6bf772 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -314,6 +314,15 @@ </element> </define>
+ <define name='sourceinfonetfscifs'>
This is equivalent to the gluster one. How about renaming the gluster one to a more generic name ... and
+ <element name='dir'> + <attribute name='path'> + <ref name='dirPath'/> + </attribute> + <empty/> + </element> + </define> + <define name='sourceinfonetfsgluster'> <element name='dir'> <attribute name='path'> @@ -400,7 +409,6 @@ <choice> <value>auto</value> <value>nfs</value> - <value>cifs</value> </choice> </attribute> </element> @@ -488,6 +496,20 @@ <group> <interleave> <ref name='sourceinfohost'/> + <ref name='sourceinfonetfscifs'/> + <element name='format'> + <attribute name='type'> + <value>cifs</value>
since the whole block is identical you could just add <choice> <value>cifs</value> <value>glusterfs</value> </choice> here to avoid duplicating the whole section.
+ </attribute> + </element> + <optional> + <ref name='sourceinfovendor'/> + </optional> + </interleave> + </group> + <group> + <interleave> + <ref name='sourceinfohost'/> <ref name='sourceinfonetfsgluster'/> <element name='format'> <attribute name='type'> diff --git a/tests/storagepoolxml2xmlin/pool-netfs-cifs.xml b/tests/storagepoolxml2xmlin/pool-netfs-cifs.xml
The test section looks good to me. ACK if you avoid the duplication. 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 | 29 ++++++++++++++++++++++++----- 3 files changed, 31 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 337b8d3..6ba698c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -388,6 +388,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; @@ -427,11 +429,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; @@ -453,6 +461,17 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) "direct-io-mode=1", pool->def->target.path, NULL); + else if (cifsfs) + cmd = virCommandNewArgList(MOUNT, + "-t", + (pool->def->type == VIR_STORAGE_POOL_FS ? + virStoragePoolFormatFileSystemTypeToString(pool->def->source.format) : + virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format)), + src, + pool->def->target.path, + "-o", + "guest", + NULL); else cmd = virCommandNewArgList(MOUNT, "-t", -- 2.1.0
On Wed, Jun 03, 2015 at 13:06:58 -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 | 29 ++++++++++++++++++++++++----- 3 files changed, 31 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 337b8d3..6ba698c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -388,6 +388,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);
@cifsfs can be true only if pool->def->type == VIR_STORAGE_POOL_NETFS.
virCommandPtr cmd = NULL; int ret = -1; int rc; @@ -427,11 +429,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; @@ -453,6 +461,17 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) "direct-io-mode=1", pool->def->target.path, NULL); + else if (cifsfs) + cmd = virCommandNewArgList(MOUNT, + "-t", + (pool->def->type == VIR_STORAGE_POOL_FS ?
... so the first part of this ternary can't ever be true here.
+ virStoragePoolFormatFileSystemTypeToString(pool->def->source.format) : + virStoragePoolFormatFileSystemNetTypeToString(pool->def->source.format)), + src, + pool->def->target.path, + "-o", + "guest", + NULL); else cmd = virCommandNewArgList(MOUNT, "-t",
Peter
On 06/03/2015 01:06 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1186969
The first patch follows a previous change to fix/adjust the gluster specific rng formatting
The second patch resolves the particular issue ensuring to generate the source path with the "//%s/%s" instead of "%s:%s" and with the "-o group" to connect to the Samba serbver.
John Ferlan (2): storage: Fix the schema and add tests for cifs pool storage: Generate correct parameters for CIFS
docs/formatstorage.html.in | 7 ++++-- docs/schemas/storagepool.rng | 24 +++++++++++++++++++- docs/storage.html.in | 3 ++- src/storage/storage_backend_fs.c | 29 ++++++++++++++++++++----- tests/storagepoolxml2xmlin/pool-netfs-cifs.xml | 12 ++++++++++ tests/storagepoolxml2xmlout/pool-netfs-cifs.xml | 15 +++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-cifs.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-cifs.xml
Ping... I know... Samba/Windows... no one's complained it's broken from day 1... Thanks - John
participants (2)
-
John Ferlan -
Peter Krempa