[libvirt] [PATCH 0/3] Allow adding mountOpts to the storage pool mount command
Modify the generation of the command line to allow usage of a new XML pool source directory "mount_opts" in order to allow (for instance) starting an NFS pool with specific mount options. John Ferlan (3): storage: Add mount options attribute for pool source dir element storage: Add mountOpts to the storage pool mount command line virsh: Add source-mount-opts for pool commands docs/formatstorage.html.in | 8 ++++++++ docs/schemas/storagepool.rng | 5 +++++ src/conf/storage_conf.c | 12 +++++++++-- src/conf/storage_conf.h | 3 +++ src/storage/storage_util.c | 4 ++++ .../pool-netfs-mountopts.argv | 1 + tests/storagepoolxml2argvtest.c | 1 + .../pool-netfs-mountopts.xml | 20 +++++++++++++++++++ .../pool-netfs-mountopts.xml | 20 +++++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + tools/virsh-pool.c | 15 +++++++++++--- tools/virsh.pod | 5 +++++ 12 files changed, 90 insertions(+), 5 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml -- 2.17.2
If libvirt creates the command to mount the storage pool, it should be able to provide a list of mount options per the policy of the consumer. Add the "mount_opts" RNG/XML variable and the corresponding _virStoragePoolSource field to take whatever is provided in the XML to use in order to create the command. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 8 ++++++++ docs/schemas/storagepool.rng | 5 +++++ src/conf/storage_conf.c | 12 +++++++++-- src/conf/storage_conf.h | 3 +++ .../pool-netfs-mountopts.xml | 20 +++++++++++++++++++ .../pool-netfs-mountopts.xml | 20 +++++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index b6bf3edbd2..491d75c2b1 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -155,6 +155,14 @@ 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> + + <p>An optional <code>mount_opts</code> may be provided to add + mount options for pools such as <code>netfs</code> in order to + provide a list of mount options to be provided to the mount command + via the "-o" option during pool startup or creation. The options + are passed directly via a string to the command. + <span class="since">Since 5.0.0.</span> + </p> <dt><code>adapter</code></dt> <dd>Provides the source for pools backed by SCSI adapters (pool type <code>scsi</code>). May only occur once. diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 74f4363106..7252d8d44b 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -354,6 +354,11 @@ <attribute name='path'> <ref name='absDirPath'/> </attribute> + <optional> + <attribute name='mount_opts'> + <text/> + </attribute> + </optional> <empty/> </element> </define> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 55db7a96f5..56710395be 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -372,6 +372,7 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) virStoragePoolSourceDeviceClear(&source->devices[i]); VIR_FREE(source->devices); VIR_FREE(source->dir); + VIR_FREE(source->mountOpts); VIR_FREE(source->name); virStorageAdapterClear(&source->adapter); virStorageSourceInitiatorClear(&source->initiator); @@ -527,6 +528,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, VIR_STRDUP(source->dir, "/") < 0) goto cleanup; + /* Optional mount options for the directory */ + source->mountOpts = virXPathString("string(./dir/@mount_opts)", ctxt); + if ((adapternode = virXPathNode("./adapter", ctxt))) { if (virStorageAdapterParseXML(&source->adapter, adapternode, ctxt) < 0) goto cleanup; @@ -934,8 +938,12 @@ virStoragePoolSourceFormat(virBufferPtr buf, } } - if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) - virBufferEscapeString(buf, "<dir path='%s'/>\n", src->dir); + if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) { + virBufferEscapeString(buf, "<dir path='%s'", src->dir); + if (src->mountOpts) + virBufferEscapeString(buf, " mount_opts='%s'", src->mountOpts); + virBufferAddLit(buf, "/>\n"); + } if ((options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) && (src->adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST || diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index dc0aa2ab29..780a219d3f 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -175,6 +175,9 @@ struct _virStoragePoolSource { /* Or a directory */ char *dir; + /* If provided, a list of mount(8) options for mounting the pool's dir */ + char *mountOpts; + /* Or an adapter */ virStorageAdapter adapter; diff --git a/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml b/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml new file mode 100644 index 0000000000..ae312f6e5c --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml @@ -0,0 +1,20 @@ +<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' mount_opts="nodev,nosuid"/> + <format type='nfs'/> + </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-mountopts.xml b/tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml new file mode 100644 index 0000000000..3c6913d370 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-netfs-mountopts.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' mount_opts='nodev,nosuid'/> + <format type='nfs'/> + </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 707d09f5c2..08d9a08020 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -85,6 +85,7 @@ mymain(void) DO_TEST("pool-netfs-auto"); DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); + DO_TEST("pool-netfs-mountopts"); DO_TEST("pool-scsi"); DO_TEST("pool-scsi-type-scsi-host"); DO_TEST("pool-scsi-type-fc-host"); -- 2.17.2
Add the mountOpts to the generated backend mount command line for the storage pool. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 4 ++++ tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv | 1 + tests/storagepoolxml2argvtest.c | 1 + 3 files changed, 6 insertions(+) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a84ee5b600..6d7c01e425 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4336,6 +4336,10 @@ virStorageBackendFileSystemMountCmd(const char *cmdstr, virStorageBackendFileSystemMountCIFSArgs(cmd, src, def); else virStorageBackendFileSystemMountDefaultArgs(cmd, src, def); + + if (def->source.mountOpts) + virCommandAddArgList(cmd, "-o", def->source.mountOpts, NULL); + return cmd; } diff --git a/tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv b/tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv new file mode 100644 index 0000000000..16d35bc175 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv @@ -0,0 +1 @@ +mount -t nfs localhost:/var/lib/libvirt/images /mnt -o nodev,nosuid diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index 2f2d40e027..0df97fb390 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -159,6 +159,7 @@ mymain(void) DO_TEST("pool-netfs-auto"); DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); + DO_TEST("pool-netfs-mountopts"); DO_TEST_FAIL("pool-scsi"); DO_TEST_FAIL("pool-scsi-type-scsi-host"); DO_TEST_FAIL("pool-scsi-type-fc-host"); -- 2.17.2
Add the ability to add the mount options for the pool on the virsh command line for pool-{create|define}-as commands. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-pool.c | 15 ++++++++++++--- tools/virsh.pod | 5 +++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 75ec572af2..8c14de97f1 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -141,6 +141,10 @@ {.name = "adapter-parent-fabric-wwn", \ .type = VSH_OT_STRING, \ .help = N_("adapter parent scsi_hostN fabric_wwn to be used for underlying vHBA storage") \ + }, \ + {.name = "source-mount-opts", \ + .type = VSH_OT_STRING, \ + .help = N_("source mount options for an nfs pool's source-path") \ } virStoragePoolPtr @@ -324,7 +328,7 @@ virshBuildPoolXML(vshControl *ctl, *secretUsage = NULL, *adapterName = NULL, *adapterParent = NULL, *adapterWwnn = NULL, *adapterWwpn = NULL, *secretUUID = NULL, *adapterParentWwnn = NULL, *adapterParentWwpn = NULL, - *adapterParentFabricWwn = NULL; + *adapterParentFabricWwn = NULL, *mountOpts = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; VSH_EXCLUSIVE_OPTIONS("secret-usage", "secret-uuid"); @@ -336,6 +340,7 @@ virshBuildPoolXML(vshControl *ctl, if (vshCommandOptStringReq(ctl, cmd, "source-host", &srcHost) < 0 || vshCommandOptStringReq(ctl, cmd, "source-path", &srcPath) < 0 || + vshCommandOptStringReq(ctl, cmd, "source-mount-opts", &mountOpts) < 0 || vshCommandOptStringReq(ctl, cmd, "source-dev", &srcDev) < 0 || vshCommandOptStringReq(ctl, cmd, "source-name", &srcName) < 0 || vshCommandOptStringReq(ctl, cmd, "source-format", &srcFormat) < 0 || @@ -363,8 +368,12 @@ virshBuildPoolXML(vshControl *ctl, if (srcHost) virBufferAsprintf(&buf, "<host name='%s'/>\n", srcHost); - if (srcPath) - virBufferAsprintf(&buf, "<dir path='%s'/>\n", srcPath); + if (srcPath) { + virBufferAsprintf(&buf, "<dir path='%s'", srcPath); + if (mountOpts) + virBufferAsprintf(&buf, " mount_opts='%s'", mountOpts); + virBufferAddLit(&buf, "/>\n"); + } if (srcDev) virBufferAsprintf(&buf, "<device path='%s'/>\n", srcDev); if (adapterWwnn && adapterWwpn) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 86a4996cae..aa40c580ab 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3884,6 +3884,7 @@ just I<--build> is provided, then B<pool-build> is called with no flags. =item B<pool-create-as> I<name> I<type> [I<--source-host hostname>] [I<--source-path path>] [I<--source-dev path>] +[I<--source-mount-opts mountOpts>] [I<--source-name name>] [I<--target path>] [I<--source-format format>] [I<--auth-type authtype> I<--auth-username username> [I<--secret-usage usage> | I<--secret-uuid uuid>]] @@ -3910,6 +3911,10 @@ gluster). [I<--source-path path>] provides the source directory path for pools backed by directories (pool type dir). +[<--source-mount-opts mountOpts>] provides a string to be used when creating +the mount command for a netfs type pool. The options should be in a comma +separated list format as described by the mount options command. + [I<--source-dev path>] provides the source path for pools backed by physical devices (pool types fs, logical, disk, iscsi, zfs). -- 2.17.2
On Tue, Dec 18, 2018 at 03:03:14PM -0500, John Ferlan wrote:
Modify the generation of the command line to allow usage of a new XML pool source directory "mount_opts" in order to allow (for instance) starting an NFS pool with specific mount options.
We should not try to pretend to support passing arbitrary options via XML. See the thread from the last time this was proposed: https://www.redhat.com/archives/libvir-list/2014-May/msg00938.html https://www.redhat.com/archives/libvir-list/2014-June/msg00188.html Jano
On 12/21/18 6:16 AM, Ján Tomko wrote:
On Tue, Dec 18, 2018 at 03:03:14PM -0500, John Ferlan wrote:
Modify the generation of the command line to allow usage of a new XML pool source directory "mount_opts" in order to allow (for instance) starting an NFS pool with specific mount options.
We should not try to pretend to support passing arbitrary options via XML.
See the thread from the last time this was proposed: https://www.redhat.com/archives/libvir-list/2014-May/msg00938.html https://www.redhat.com/archives/libvir-list/2014-June/msg00188.html
Jano
sigh... But in a way we already do allow passing arbitrary options via XML in the form of <os> {<initarg>, <initenv>, and/or <cmdline>} </os> and/or <bootloader_args>. There's also a free form <lease> <lockspace> and/or <key> although that's a bit different. In Daniel's response: https://www.redhat.com/archives/libvir-list/2014-June/msg00188.html "... The only way I'd support passthrough is if it were done in he same way as QEMU passthrough where it used a separate XML namespace which was clearly marked "use at your own risk, unsupported if it breaks". " the "unsupported" part would seem to be 'undesirable' at least w/r/t what's being asked for from the (private) bz. The request is "Shouldn't the security options for nodev,nosuid or even noexec be available via KVM?" (as it relates to the 'mount ... -o <options>' command). So if we say unsupported, then for those customers that desire perhaps higher security I would think/believe that they would want something that is supported. TBH: The XML namespace option used by QEMU commandline args and for LXC sharenet, shareipc, & shareuts options would seem to be a bit of overkill for what amounts to a more "bounded" operation trying to add free form (to a degree) options for a specific storage backend command. This isn't some new option that we haven't had the time to implement in libvirt for QEMU domains - it's arguments to an OS command. I'll dig a little more on this, but figured I'd throw this out there as something to consider. I guess I'm not seeing the overall value of adding yards of code. Then there's the quandary of this really is for storage source, but following the domain model I think it'd look odd to have it as the same level as target too. How would it be handled if someone wanted some sort of name/value for <target>? Right now there's two known consumers - netfs for the mount -o args and rbd to allow changing the defaults of (currently) 3 name/value config arguments. Plus perhaps a bunch of debugging uses that weren't defined. John
participants (2)
-
John Ferlan -
Ján Tomko