[libvirt] [PATCH v3 00/11] Allow adding mountOpts to the storage pool mount command
v2: https://www.redhat.com/archives/libvir-list/2019-January/msg00164.html Changes since v2: * Split out the noexec, nodev, nosuid, and (new) ro into the first 3 patches as "known" or "supported" mount options that can be added. I avoided just adding them noexec, nodev, and nosuid by default. * Add patches 4-6 to essentially support the nfsvers option * The rest of the patches are reworked to add support for namespace at the pool level (as opposed to the v2 at the source level). John Ferlan (11): conf: Add some defined NFS Storage Pool mount options storage: Add mountOpts to the storage pool mount command line virsh: Add source-mount-opts for pool commands conf: Add optional NFS Source Pool <protocol ver='n'/> option storage: Add the nfsvers to the command line virsh: Add source-protocol-ver for pool commands conf: Introduce virStoragePoolXMLNamespace nfs: Add infrastructure to manage XML namespace options docs,tests: Add schema, description, and tests for NFS namespace storage: Add NFS storage pool namespace options to command line rbd: Utilize storage pool namespace to manage config options docs/formatstorage.html.in | 137 +++++++++++++++ docs/schemas/storagepool.rng | 70 ++++++++ src/conf/storage_conf.c | 120 ++++++++++++- src/conf/storage_conf.h | 40 +++++ src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 128 ++++++++++++++ src/storage/storage_backend_rbd.c | 166 +++++++++++++++++- src/storage/storage_util.c | 47 +++++ src/storage/storage_util.h | 14 ++ tests/Makefile.am | 4 +- .../pool-netfs-mountopts.argv | 1 + .../pool-netfs-ns-mountopts.argv | 2 + .../pool-netfs-protocol-ver.argv | 1 + tests/storagepoolxml2argvtest.c | 6 + .../pool-netfs-mountopts.xml | 24 +++ .../pool-netfs-ns-mountopts.xml | 29 +++ .../pool-netfs-protocol-ver.xml | 21 +++ .../pool-rbd-ns-configopts.xml | 17 ++ .../pool-netfs-mountopts.xml | 24 +++ .../pool-netfs-ns-mountopts.xml | 29 +++ .../pool-netfs-protocol-ver.xml | 21 +++ .../pool-rbd-ns-configopts.xml | 20 +++ tests/storagepoolxml2xmltest.c | 9 + tools/virsh-pool.c | 44 ++++- tools/virsh.pod | 10 ++ 25 files changed, 977 insertions(+), 8 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-ns-mountopts.argv create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-protocol-ver.argv create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-ns-mountopts.xml create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-protocol-ver.xml create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-ns-configopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-ns-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-protocol-ver.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-ns-configopts.xml -- 2.20.1
https://bugzilla.redhat.com/show_bug.cgi?id=1584663 Add the ability to parse/manage some defined NFS Storage Pool mount options. Keep the set of defined options limited to noexec, nosuid, nodev, and ro. Subsequent patches will add the ability to provide the NFS nfsvers value and eventally any option via storage pool namespaces. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 21 +++++++++ docs/schemas/storagepool.rng | 20 ++++++++ src/conf/storage_conf.c | 47 +++++++++++++++++++ src/conf/storage_conf.h | 13 +++++ .../pool-netfs-mountopts.xml | 24 ++++++++++ .../pool-netfs-mountopts.xml | 24 ++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 150 insertions(+) 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..97c90f0797 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -121,6 +121,19 @@ </source> ...</pre> + <pre> +... + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + <format type='nfs'/> + <mount_opts> + <option name='nodev'/> + <option name='nosuid'/> + </mount_opts> + </source> +...</pre> + <dl> <dt><code>device</code></dt> <dd>Provides the source for pools backed by physical devices @@ -386,6 +399,14 @@ LVM metadata type. All drivers are required to have a default value for this, so it is optional. <span class="since">Since 0.4.1</span></dd> + <dt><code>mount_opts</code></dt> + <dd>Provide an optional set of mount options for the <code>netfs</code> + pool startup or creation to be provided via the "-o" option. + The desired options are specified by using the subelement + <code>option</code> with the attribute <code>name</code> to + provide the option. Supported option names are "noexec", "nosuid", + "nodev", and "ro". + <span class="since">Since 5.1.0</span></dd> <dt><code>vendor</code></dt> <dd>Provides optional information about the vendor of the storage device. This contains a single diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 74f4363106..d52856b556 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -531,6 +531,9 @@ <ref name='sourceinfohost'/> <ref name='sourceinfodir'/> <ref name='sourcefmtnetfs'/> + <optional> + <ref name='sourcemountopts'/> + </optional> <optional> <ref name='sourceinfovendor'/> </optional> @@ -576,6 +579,23 @@ </element> </define> + <define name='sourcemountopts'> + <element name='mount_opts'> + <zeroOrMore> + <element name='option'> + <attribute name='name'> + <choice> + <value>noexec</value> + <value>nosuid</value> + <value>nodev</value> + <value>ro</value> + </choice> + </attribute> + </element> + </zeroOrMore> + </element> + </define> + <define name='sourcedisk'> <element name='source'> <interleave> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 55db7a96f5..aafb7980e7 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -70,6 +70,10 @@ VIR_ENUM_IMPL(virStoragePoolFormatFileSystemNet, VIR_STORAGE_POOL_NETFS_LAST, "auto", "nfs", "glusterfs", "cifs") +VIR_ENUM_IMPL(virStoragePoolSourceMountOpts, + VIR_STORAGE_POOL_SOURCE_MOUNT_OPTS_LAST, + "noexec", "nosuid", "nodev", "ro") + VIR_ENUM_IMPL(virStoragePoolFormatDisk, VIR_STORAGE_POOL_DISK_LAST, "unknown", "dos", "dvh", "gpt", @@ -378,6 +382,10 @@ virStoragePoolSourceClear(virStoragePoolSourcePtr source) virStorageAuthDefFree(source->auth); VIR_FREE(source->vendor); VIR_FREE(source->product); + for (i = 0; i < source->nmountOpts; i++) + VIR_FREE(source->mountOpts[i]); + VIR_FREE(source->mountOpts); + source->nmountOpts = 0; } @@ -546,6 +554,33 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, authdef = NULL; } + /* Optional, but defined mount ops */ + VIR_FREE(nodeset); + n = virXPathNodeSet("./mount_opts/option", ctxt, &nodeset); + if (n < 0) + goto cleanup; + + if (n > 0) { + if (VIR_ALLOC_N(source->mountOpts, n) < 0) + goto cleanup; + for (i = 0; i < n; i++) { + if (!(source->mountOpts[i] = virXMLPropString(nodeset[i], + "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no mount option name specified")); + goto cleanup; + } + source->nmountOpts++; + /* Do we recognize the string? */ + if (virStoragePoolSourceMountOptsTypeFromString(source->mountOpts[i]) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown mount option name '%s' specified"), + source->mountOpts[i]); + goto cleanup; + } + } + } + source->vendor = virXPathString("string(./vendor/@name)", ctxt); source->product = virXPathString("string(./product/@name)", ctxt); @@ -962,6 +997,18 @@ virStoragePoolSourceFormat(virBufferPtr buf, if (src->auth) virStorageAuthDefFormat(buf, src->auth); + if (src->mountOpts) { + virBufferAddLit(buf, "<mount_opts>\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < src->nmountOpts; i++) + virBufferEscapeString(buf, "<option name='%s'/>\n", + src->mountOpts[i]); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</mount_opts>\n"); + } + virBufferEscapeString(buf, "<vendor name='%s'/>\n", src->vendor); virBufferEscapeString(buf, "<product name='%s'/>\n", src->product); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index dc0aa2ab29..c23e6720ac 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -193,6 +193,10 @@ struct _virStoragePoolSource { /* Product name of the source*/ char *product; + /* Defined mount options to be added to mount command line */ + char **mountOpts; + size_t nmountOpts; + /* Pool type specific format such as filesystem type, * or lvm version, etc. */ @@ -332,6 +336,15 @@ typedef enum { } virStoragePoolFormatFileSystemNet; VIR_ENUM_DECL(virStoragePoolFormatFileSystemNet) +typedef enum { + VIR_STORAGE_POOL_SOURCE_MOUNT_OPTS_NOEXEC = 0, + VIR_STORAGE_POOL_SOURCE_MOUNT_OPTS_NOSUID, + VIR_STORAGE_POOL_SOURCE_MOUNT_OPTS_NODEV, + VIR_STORAGE_POOL_SOURCE_MOUNT_OPTS_RO, + VIR_STORAGE_POOL_SOURCE_MOUNT_OPTS_LAST, +} virStoragePoolSourceMountOpts; +VIR_ENUM_DECL(virStoragePoolSourceMountOpts) + typedef enum { VIR_STORAGE_POOL_DISK_UNKNOWN = 0, VIR_STORAGE_POOL_DISK_DOS = 1, diff --git a/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml b/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml new file mode 100644 index 0000000000..c2bb3bf4ed --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml @@ -0,0 +1,24 @@ +<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'/> + <format type='nfs'/> + <mount_opts> + <option name='nodev'/> + <option name='nosuid'/> + </mount_opts> + </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..786acf8887 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml @@ -0,0 +1,24 @@ +<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='nfs'/> + <mount_opts> + <option name='nodev'/> + <option name='nosuid'/> + </mount_opts> + </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.20.1
On Tue, Jan 15, 2019 at 08:09:12PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1584663
Add the ability to parse/manage some defined NFS Storage Pool mount options. Keep the set of defined options limited to noexec, nosuid, nodev, and ro. Subsequent patches will add the ability to provide the NFS nfsvers value and eventally any option via storage pool namespaces.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 21 +++++++++ docs/schemas/storagepool.rng | 20 ++++++++ src/conf/storage_conf.c | 47 +++++++++++++++++++ src/conf/storage_conf.h | 13 +++++ .../pool-netfs-mountopts.xml | 24 ++++++++++ .../pool-netfs-mountopts.xml | 24 ++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 150 insertions(+) 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..97c90f0797 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -121,6 +121,19 @@ </source> ...</pre>
+ <pre> +... + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + <format type='nfs'/> + <mount_opts> + <option name='nodev'/> + <option name='nosuid'/> + </mount_opts> + </source> +...</pre> + <dl> <dt><code>device</code></dt> <dd>Provides the source for pools backed by physical devices @@ -386,6 +399,14 @@ LVM metadata type. All drivers are required to have a default value for this, so it is optional. <span class="since">Since 0.4.1</span></dd>
+ <dt><code>mount_opts</code></dt> + <dd>Provide an optional set of mount options for the <code>netfs</code> + pool startup or creation to be provided via the "-o" option. + The desired options are specified by using the subelement + <code>option</code> with the attribute <code>name</code> to + provide the option. Supported option names are "noexec", "nosuid", + "nodev", and "ro". + <span class="since">Since 5.1.0</span></dd>
Hmm, this has gone back to the "Mount options" concept from v1 which I don't think is appropriate. In v2 feedback I had suggested that we should have some more explicit XML description for the subset of options we wish to support, with each option potentially using a different syntax, no generic mount options syntax. For 'ro' we should use our normal <readonly/> empty element syntax. For nosetuid, nodev, noexec, I'm still inclined to say we should enable them by default. Despite it being a semantic change, I think it is the right thing for a volume that is being used for storing VM disk images. If we did want it in the XML though, I think it should be via a <features> element, not <mount_opts>, as the inverse - iow opt-in to the insecure behaviour <features> <allow-suid/> <allow-devnode/> <allow-exec/> </features> This provides a generic framework that will work for other storage pool backend features where there's no mount concept involved eg LVM, iSCSI, etc Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/16/19 7:00 AM, Daniel P. Berrangé wrote:
On Tue, Jan 15, 2019 at 08:09:12PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1584663
Add the ability to parse/manage some defined NFS Storage Pool mount options. Keep the set of defined options limited to noexec, nosuid, nodev, and ro. Subsequent patches will add the ability to provide the NFS nfsvers value and eventally any option via storage pool namespaces.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 21 +++++++++ docs/schemas/storagepool.rng | 20 ++++++++ src/conf/storage_conf.c | 47 +++++++++++++++++++ src/conf/storage_conf.h | 13 +++++ .../pool-netfs-mountopts.xml | 24 ++++++++++ .../pool-netfs-mountopts.xml | 24 ++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 150 insertions(+) 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..97c90f0797 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -121,6 +121,19 @@ </source> ...</pre>
+ <pre> +... + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + <format type='nfs'/> + <mount_opts> + <option name='nodev'/> + <option name='nosuid'/> + </mount_opts> + </source> +...</pre> + <dl> <dt><code>device</code></dt> <dd>Provides the source for pools backed by physical devices @@ -386,6 +399,14 @@ LVM metadata type. All drivers are required to have a default value for this, so it is optional. <span class="since">Since 0.4.1</span></dd>
+ <dt><code>mount_opts</code></dt> + <dd>Provide an optional set of mount options for the <code>netfs</code> + pool startup or creation to be provided via the "-o" option. + The desired options are specified by using the subelement + <code>option</code> with the attribute <code>name</code> to + provide the option. Supported option names are "noexec", "nosuid", + "nodev", and "ro". + <span class="since">Since 5.1.0</span></dd>
Hmm, this has gone back to the "Mount options" concept from v1 which I don't think is appropriate. In v2 feedback I had suggested that we should have some more explicit XML description for the subset of options we wish to support, with each option potentially using a different syntax, no generic mount options syntax.
In v1 it seems to me at least the objection was arbitrarily named mount_opts, so specifically named mount_opts felt like a fair trade-off. In v2 you noted the features element to be similar to the domain features, but then pointed out that you felt we could just default to using them. A followup pointed out one concern over OS syntax differences, which is essentially why I took this approach - force someone to provide the option. If that option used different syntax on some other OS, then someone else who cares about that OS could send a patch to change the name generated (and we avoid making any assumptions).
For 'ro' we should use our normal <readonly/> empty element syntax.
True we could, but believe it or not readonly is not even defined in storagepool.rng. It's a domaincommon.rng to be used for hypervisor options. Adding ro to the mount_opts list was essentially a shortcut of patches to include ro with the others. I saw nfsvers a bit different since it would provide more than a boolean option. Still beyond NFS I'm trying to envision how we'd supply readonly such that it would matter for the pool. Wouldn't networked pools require the server side to have set up readonly? For local pools how much of the exposure of the volume would be controllable such that the pool could manage the readonly-ness? A virDomainDiskTranslateSourcePool "option" I suppose that could be associated with the domain readonly attribute. Perhaps ro or readonly should just be separate then because it involves a lot more interaction. Does the overhead of code addition outweigh the usefulness of a generic pool readonly option? No one's asked for such a feature, so sneaking in ro to/for an NFS mount option felt reasonable.
For nosetuid, nodev, noexec, I'm still inclined to say we should enable them by default. Despite it being a semantic change, I think it is the right thing for a volume that is being used for storing VM disk images.
Again, the fear of doing that is some OS has different syntax (from your v2 response it's possible that FreeBSD mount command options may differ from Linux mount command options). So, using named options seemed to be a better tradeoff than just altering the command generation to add those options for NFS mounts. I'm not deadset against altering the defaults we set. It's certainly the easiest approach. Drop patch 1 and 3... Changes patch 2 to a very static operation, changing the existing .args output for NFS pools, and still leaves patches 4-11 mostly unchanged other than to "merge" the changes from patch 2.
If we did want it in the XML though, I think it should be via a <features> element, not <mount_opts>, as the inverse - iow opt-in to the insecure behaviour
<features> <allow-suid/> <allow-devnode/> <allow-exec/> </features>
I really don't see the point of differently named XML. As much as you don't seem to like mount_opts I'm not a fan of features. At least with mount_opts it's obvious what the use is. I just don't see mount options as features in the same light as hypervisor features. It's a command option not a feature for the pool. Other pools use specifically named elements to support their specific features/needs <name>, <host>, <adapter>, <auth>, <initiator>, etc. But something that is very specific to how the NFS is being used is required to use a new, generic "features" element. If by defaulting to nosuid, nodev, and noexec we run into trouble with some other OS, then that requires every pool on that OS to be modified to add these because we changed the default. That seems counter to other examples where we force to opt-in. Although because these are security related, I can see an argument for defaulting to them other than of course that NFS doesn't default that way. Rock and hard place. FWIW: AIUI, the default for the listed option options is the inverse. That is "suid" is the default for NFS... Likewise, "dev", "exec", and "rw". So supplying "suid" instead of "nosuid" doesn't mean anything since "suid" is the default. It does mean though code to "remove" default options which in a way is no different than code to supply an option that isn't default. Restated - knowing that "suid" is the default, we don't supply it. Of course we could, but that hasn't been the norm thus far. We just take the NFS defaults. Still if this were to follow the domain model, then why wouldn't it be: <features> <netfs> <option name='suid' state='{on|off}'/> <option name='exec' state='{on|off}'/> ... (or <start_option .../> since this isn't create, define, build, destroy, or undefine) I would think that would be far more confusing, but seems to me to follow how features are assigned/used for specific hypervisors. It still requires some sort of mapping of names and we're not allowing arbitrary ones. Does nosuid, noexec, or nodev have meaning beyond that of the meaning used for NFS? If the answer is no, then it's a very specific feature to a very specific pool.
This provides a generic framework that will work for other storage pool backend features where there's no mount concept involved eg LVM, iSCSI, etc
And what "features" would we consider allowing without having to craft a different backend? Allowing some provided option for LVM commands could result in needing to write code that would be able to handle multiple different formats. In particular I'm thinking about how thin provisioning is supported. Very different way to configure and a completely different means to scan the generated output looking for usable volumes. Previous attempts to co-mingle in the current logical backend were NACK'd and no one has taken up the create a new backend that supports thin pools vs thin snapshots (I think the latter is the terminology used, it's been a few years). For iSCSI we now have the iscsi-direct backend which perhaps could make use of something, but it'd be something fairly specific to some iSCSI feature which is no different (to me at least) than providing some specific option for netfs. John
Regards, Daniel
On Wed, Jan 16, 2019 at 12:47:32PM -0500, John Ferlan wrote:
On 1/16/19 7:00 AM, Daniel P. Berrangé wrote:
Hmm, this has gone back to the "Mount options" concept from v1 which I don't think is appropriate. In v2 feedback I had suggested that we should have some more explicit XML description for the subset of options we wish to support, with each option potentially using a different syntax, no generic mount options syntax.
In v1 it seems to me at least the objection was arbitrarily named mount_opts, so specifically named mount_opts felt like a fair trade-off.
I guess I should have been clearer that I didn't like both parts.
In v2 you noted the features element to be similar to the domain features, but then pointed out that you felt we could just default to using them. A followup pointed out one concern over OS syntax differences, which is essentially why I took this approach - force someone to provide the option. If that option used different syntax on some other OS, then someone else who cares about that OS could send a patch to change the name generated (and we avoid making any assumptions).
If we force apps to provide the option in the XML then it more likely to cause incompatibilities, as most apps won't test whether an OS supports a given option. If we default enable them in libvirt then libvirt can just put this in in ifdef __linux__ so we don't risk breaking other non-Linux OS.
For 'ro' we should use our normal <readonly/> empty element syntax.
True we could, but believe it or not readonly is not even defined in storagepool.rng. It's a domaincommon.rng to be used for hypervisor options. Adding ro to the mount_opts list was essentially a shortcut of patches to include ro with the others. I saw nfsvers a bit different since it would provide more than a boolean option.
Even though its a different schema, it is worth using a consistent element naming approach IMHO.
Still beyond NFS I'm trying to envision how we'd supply readonly such that it would matter for the pool. Wouldn't networked pools require the server side to have set up readonly? For local pools how much of the exposure of the volume would be controllable such that the pool could manage the readonly-ness? A virDomainDiskTranslateSourcePool "option" I suppose that could be associated with the domain readonly attribute.
Perhaps ro or readonly should just be separate then because it involves a lot more interaction. Does the overhead of code addition outweigh the usefulness of a generic pool readonly option? No one's asked for such a feature, so sneaking in ro to/for an NFS mount option felt reasonable.
We can't predict the future reliably. If we use <mountopts><ro/></mountopts> and then in future find a need for readonly on other storage pools where <mountopts> is not relevant, we'll have to invent a second way to specify readonly. By using <readonly/> we've not tied our hands, so we have more flexibility to cope with the future needs. It may never matter, but I'd always aim to avoid restricting ourselves.
For nosetuid, nodev, noexec, I'm still inclined to say we should enable them by default. Despite it being a semantic change, I think it is the right thing for a volume that is being used for storing VM disk images.
Again, the fear of doing that is some OS has different syntax (from your v2 response it's possible that FreeBSD mount command options may differ from Linux mount command options).
So, using named options seemed to be a better tradeoff than just altering the command generation to add those options for NFS mounts.
If we did it in defaults, then we'd have to be conservative in what we enable, so if we don't know they are supported by an OS we shouldn't enable them in the code when building for that OS. BTW, I did just check FreeBSD and it supports nosuid and noexec but not nodev. So we'd want #ifdef __linux__ #define opts ",nodev,nosuid,noexec" #else #ifdef__FreeBSD__ #define opts ",nosuid,noexec" #else #define opts "" #endif I'm thinking perhaps we should never have been considering the XML at all for nodev, nosuid, noexec. Instead it might be better as a configurable at a host level rather than XML level. eg a /etc/libvirt/storage.conf file with an 'nfs_extra_mount_opts' config parameter, whose default value is "nodev,nosuid,noexec" on Linux hosts. That would let sysadmins enforce a host-wide policy without needing to worry about whether an application has been updated to support setting the XML to add extra mount options. That would avoid most debate about mount options / features in the XML. Probably the protocol version is the only thing that is important for the XML as that can vary per-server. Read-only is also probably relevant in XML.
I would think that would be far more confusing, but seems to me to follow how features are assigned/used for specific hypervisors. It still requires some sort of mapping of names and we're not allowing arbitrary ones. Does nosuid, noexec, or nodev have meaning beyond that of the meaning used for NFS? If the answer is no, then it's a very specific feature to a very specific pool.
BTW nosuid, noexec, nodev are nothing todo with NFS - they're general options that can be set on any mount type.
This provides a generic framework that will work for other storage pool backend features where there's no mount concept involved eg LVM, iSCSI, etc
And what "features" would we consider allowing without having to craft a different backend?
Allowing some provided option for LVM commands could result in needing to write code that would be able to handle multiple different formats. In particular I'm thinking about how thin provisioning is supported. Very different way to configure and a completely different means to scan the generated output looking for usable volumes. Previous attempts to co-mingle in the current logical backend were NACK'd and no one has taken up the create a new backend that supports thin pools vs thin snapshots (I think the latter is the terminology used, it's been a few years).
For iSCSI we now have the iscsi-direct backend which perhaps could make use of something, but it'd be something fairly specific to some iSCSI feature which is no different (to me at least) than providing some specific option for netfs.
There's perhaps a need for <features> under the pool <source> to make it clear that the valid features are specific to the pool type. Then bearing in mind what I said earlier, perhaps we should just forget the entire features / mount opts idea for XML and use the host config file. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 01/16/2019 07:00 AM, Daniel P. Berrangé wrote:
On Tue, Jan 15, 2019 at 08:09:12PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1584663
Add the ability to parse/manage some defined NFS Storage Pool mount options. Keep the set of defined options limited to noexec, nosuid, nodev, and ro. Subsequent patches will add the ability to provide the NFS nfsvers value and eventally any option via storage pool namespaces.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 21 +++++++++ docs/schemas/storagepool.rng | 20 ++++++++ src/conf/storage_conf.c | 47 +++++++++++++++++++ src/conf/storage_conf.h | 13 +++++ .../pool-netfs-mountopts.xml | 24 ++++++++++ .../pool-netfs-mountopts.xml | 24 ++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 150 insertions(+) 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..97c90f0797 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -121,6 +121,19 @@ </source> ...</pre>
+ <pre> +... + <source> + <host name='localhost'/> + <dir path='/var/lib/libvirt/images'/> + <format type='nfs'/> + <mount_opts> + <option name='nodev'/> + <option name='nosuid'/> + </mount_opts> + </source> +...</pre> + <dl> <dt><code>device</code></dt> <dd>Provides the source for pools backed by physical devices @@ -386,6 +399,14 @@ LVM metadata type. All drivers are required to have a default value for this, so it is optional. <span class="since">Since 0.4.1</span></dd>
+ <dt><code>mount_opts</code></dt> + <dd>Provide an optional set of mount options for the <code>netfs</code> + pool startup or creation to be provided via the "-o" option. + The desired options are specified by using the subelement + <code>option</code> with the attribute <code>name</code> to + provide the option. Supported option names are "noexec", "nosuid", + "nodev", and "ro". + <span class="since">Since 5.1.0</span></dd>
Hmm, this has gone back to the "Mount options" concept from v1 which I don't think is appropriate. In v2 feedback I had suggested that we should have some more explicit XML description for the subset of options we wish to support, with each option potentially using a different syntax, no generic mount options syntax.
For 'ro' we should use our normal <readonly/> empty element syntax.
For nosetuid, nodev, noexec, I'm still inclined to say we should enable them by default. Despite it being a semantic change, I think it is the right thing for a volume that is being used for storing VM disk images.
If we did want it in the XML though, I think it should be via a <features> element, not <mount_opts>, as the inverse - iow opt-in to the insecure behaviour
<features> <allow-suid/> <allow-devnode/> <allow-exec/> </features>
It seems like this proposed XML was dropped for v4 but I just want to chime in: raw boolean <foo/> style options are a pain to deal with for apps. Anything like this should use the tristate <foo value='on|off'/> pattern, otherwise there's no way for XML to distinguish between 'user explicitly requested value=no' vs 'user didn't explicitly request anything' Thanks, Cole
https://bugzilla.redhat.com/show_bug.cgi?id=1584663 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 | 19 +++++++++++++++++++ .../pool-netfs-mountopts.argv | 1 + tests/storagepoolxml2argvtest.c | 1 + 3 files changed, 21 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..8fe6139682 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4336,6 +4336,25 @@ virStorageBackendFileSystemMountCmd(const char *cmdstr, virStorageBackendFileSystemMountCIFSArgs(cmd, src, def); else virStorageBackendFileSystemMountDefaultArgs(cmd, src, def); + + if (def->type == VIR_STORAGE_POOL_NETFS && def->source.mountOpts) { + size_t i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + VIR_AUTOFREE(char *) mountOpts = NULL; + + for (i = 0; i < def->source.nmountOpts; i++) + virBufferAsprintf(&buf, "%s,", def->source.mountOpts[i]); + + virBufferTrim(&buf, ",", -1); + + if (virBufferCheckError(&buf) < 0) + return NULL; + + mountOpts = virBufferContentAndReset(&buf); + + virCommandAddArgList(cmd, "-o", 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.20.1
https://bugzilla.redhat.com/show_bug.cgi?id=1584663 Add the ability to add the mount options for the pool on the virsh command line for pool-{create|define}-as commands, such as: virsh pool-define-as nfstest netfs \ --source-host localhost \ --source-path "/var/lib/libvirt/images" \ --source-format nfs \ --source-mount-opts "nodev,nosuid" \ --target "/mnt" in order to generate XML: <pool type='netfs'> <name>nfstest</name> <source> <host name='localhost'/> <dir path='/var/lib/libvirt/images'/> <format type='nfs'/> <mount_opts> <option name='nodev'/> <option name='nosuid'/> </mount_opts> </source> <target> <path>/mnt</path> </target> </pool> Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-pool.c | 35 ++++++++++++++++++++++++++++++++--- tools/virsh.pod | 6 ++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 70ca39bd3d..79b33a8885 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -136,6 +136,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_("comma separated list for NFS pool mount options") \ } virStoragePoolPtr @@ -319,7 +323,9 @@ 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; + size_t noptsList = 0; + char **optsList = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; VSH_EXCLUSIVE_OPTIONS("secret-usage", "secret-uuid"); @@ -345,7 +351,13 @@ virshBuildPoolXML(vshControl *ctl, vshCommandOptStringReq(ctl, cmd, "adapter-parent", &adapterParent) < 0 || vshCommandOptStringReq(ctl, cmd, "adapter-parent-wwnn", &adapterParentWwnn) < 0 || vshCommandOptStringReq(ctl, cmd, "adapter-parent-wwpn", &adapterParentWwpn) < 0 || - vshCommandOptStringReq(ctl, cmd, "adapter-parent-fabric-wwn", &adapterParentFabricWwn) < 0) + vshCommandOptStringReq(ctl, cmd, "adapter-parent-fabric-wwn", &adapterParentFabricWwn) < 0 || + vshCommandOptStringReq(ctl, cmd, "source-mount-opts", &mountOpts) < 0) + goto cleanup; + + if (mountOpts && + !(optsList = virStringSplitCount(mountOpts, ",", 0, &noptsList))) + goto cleanup; virBufferAsprintf(&buf, "<pool type='%s'>\n", type); @@ -394,6 +406,21 @@ virshBuildPoolXML(vshControl *ctl, if (srcName) virBufferAsprintf(&buf, "<name>%s</name>\n", srcName); + if (mountOpts) { + size_t i; + + virBufferAddLit(&buf, "<mount_opts>\n"); + virBufferAdjustIndent(&buf, 2); + + for (i = 0; i < noptsList; i++) + virBufferAsprintf(&buf, "<option name='%s'/>\n", + optsList[i]); + + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</mount_opts>\n"); + } + + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</source>\n"); } @@ -409,14 +436,16 @@ virshBuildPoolXML(vshControl *ctl, if (virBufferError(&buf)) { vshError(ctl, "%s", _("Failed to allocate XML buffer")); - return false; + goto cleanup; } + virStringListFree(optsList); *xml = virBufferContentAndReset(&buf); *retname = name; return true; cleanup: + virStringListFree(optsList); virBufferFreeAndReset(&buf); return false; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 86a4996cae..718efcb07e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3887,6 +3887,7 @@ just I<--build> is provided, then B<pool-build> is called with no flags. [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>]] +[I<--source-mount-opts mountOpts>] [[I<--adapter-name name>] | [I<--adapter-wwnn> wwnn I<--adapter-wwpn> wwpn] [I<--adapter-parent parent> | I<--adapter-parent-wwnn parent_wwnn> I<adapter-parent-wwpn parent_wwpn> | @@ -3929,6 +3930,11 @@ the storage pool. The I<authtype> is either chap for iscsi I<type> pools or ceph for rbd I<type> pools. Either the secret I<usage> or I<uuid> value may be provided, but not both. +[I<--source-mount-opts mountOpts>] provides a string to be used when creating +the mount command for a netfs type pool. The I<mountOpts> must be in a comma +separated list format. Valid I<mountOpts> that may be provided are "nosuid", +"noexec", "nodev", and "ro". + [I<--adapter-name name>] defines the scsi_hostN adapter name to be used for the scsi_host adapter type pool. -- 2.20.1
Add an optional way to define which NFS Server version will be used to content the target NFS server. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 7 ++++++ docs/schemas/storagepool.rng | 7 ++++++ src/conf/storage_conf.c | 22 +++++++++++++++++++ src/conf/storage_conf.h | 3 +++ .../pool-netfs-protocol-ver.xml | 21 ++++++++++++++++++ .../pool-netfs-protocol-ver.xml | 21 ++++++++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 7 files changed, 82 insertions(+) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-protocol-ver.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-protocol-ver.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 97c90f0797..58f5eb2e2e 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -127,6 +127,7 @@ <host name='localhost'/> <dir path='/var/lib/libvirt/images'/> <format type='nfs'/> + <protocol ver='3'/> <mount_opts> <option name='nodev'/> <option name='nosuid'/> @@ -399,6 +400,12 @@ LVM metadata type. All drivers are required to have a default value for this, so it is optional. <span class="since">Since 0.4.1</span></dd> + <dt><code>protocol</code></dt> + <dd>For a <code>netfs</code> Storage Pool provide a mechanism to + define which NFS protocol version number will be used to contact + the server's NFS service. The attribute <code>ver</code> accepts + an unsigned integer as the version number to use. + <span class="since">Since 5.1.0</span></dd> <dt><code>mount_opts</code></dt> <dd>Provide an optional set of mount options for the <code>netfs</code> pool startup or creation to be provided via the "-o" option. diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index d52856b556..a32c89b94c 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -531,6 +531,13 @@ <ref name='sourceinfohost'/> <ref name='sourceinfodir'/> <ref name='sourcefmtnetfs'/> + <optional> + <element name='protocol'> + <attribute name='ver'> + <ref name='unsignedInt'/> + </attribute> + </element> + </optional> <optional> <ref name='sourcemountopts'/> </optional> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index aafb7980e7..8ec360e4a1 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -428,6 +428,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virStorageAuthDefPtr authdef = NULL; char *name = NULL; char *port = NULL; + char *ver = NULL; int n; relnode = ctxt->node; @@ -554,6 +555,24 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, authdef = NULL; } + /* Option protocol version string (NFSvN) */ + if ((ver = virXPathString("string(./protocol/@ver)", ctxt))) { + if ((source->format != VIR_STORAGE_POOL_NETFS_NFS) && + (source->format != VIR_STORAGE_POOL_NETFS_AUTO)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("storage pool protocol ver unsupported for " + "pool type '%s'"), + virStoragePoolFormatFileSystemNetTypeToString(source->format)); + goto cleanup; + } + if (virStrToLong_uip(ver, NULL, 0, &source->protocolVer) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("storage pool protocol ver '%s' is malformaed"), + ver); + goto cleanup; + } + } + /* Optional, but defined mount ops */ VIR_FREE(nodeset); n = virXPathNodeSet("./mount_opts/option", ctxt, &nodeset); @@ -997,6 +1016,9 @@ virStoragePoolSourceFormat(virBufferPtr buf, if (src->auth) virStorageAuthDefFormat(buf, src->auth); + if (src->protocolVer) + virBufferAsprintf(buf, "<protocol ver='%u'/>\n", src->protocolVer); + if (src->mountOpts) { virBufferAddLit(buf, "<mount_opts>\n"); virBufferAdjustIndent(buf, 2); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index c23e6720ac..f36d4daa3c 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -201,6 +201,9 @@ struct _virStoragePoolSource { * or lvm version, etc. */ int format; + + /* Protocol version value for netfs */ + unsigned int protocolVer; }; typedef struct _virStoragePoolTarget virStoragePoolTarget; diff --git a/tests/storagepoolxml2xmlin/pool-netfs-protocol-ver.xml b/tests/storagepoolxml2xmlin/pool-netfs-protocol-ver.xml new file mode 100644 index 0000000000..40f3f94e41 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-netfs-protocol-ver.xml @@ -0,0 +1,21 @@ +<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'/> + <format type='nfs'/> + <protocol ver='3'/> + </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-protocol-ver.xml b/tests/storagepoolxml2xmlout/pool-netfs-protocol-ver.xml new file mode 100644 index 0000000000..5fcad1305b --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-netfs-protocol-ver.xml @@ -0,0 +1,21 @@ +<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='nfs'/> + <protocol ver='3'/> + </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 08d9a08020..ecdc2715b0 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -83,6 +83,7 @@ mymain(void) DO_TEST("pool-iscsi-auth"); DO_TEST("pool-netfs"); DO_TEST("pool-netfs-auto"); + DO_TEST("pool-netfs-protocol-ver"); DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); DO_TEST("pool-netfs-mountopts"); -- 2.20.1
If protocolVer present, add the -o nfsvers=# to the command line for the NFS Storage Pool Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 6 +++++- tests/storagepoolxml2argvdata/pool-netfs-protocol-ver.argv | 1 + tests/storagepoolxml2argvtest.c | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-protocol-ver.argv diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 8fe6139682..0c52b797fc 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4337,11 +4337,15 @@ virStorageBackendFileSystemMountCmd(const char *cmdstr, else virStorageBackendFileSystemMountDefaultArgs(cmd, src, def); - if (def->type == VIR_STORAGE_POOL_NETFS && def->source.mountOpts) { + if (def->type == VIR_STORAGE_POOL_NETFS && + (def->source.mountOpts || (def->source.protocolVer > 0))) { size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; VIR_AUTOFREE(char *) mountOpts = NULL; + if (def->source.protocolVer > 0) + virBufferAsprintf(&buf, "nfsvers=%u,", def->source.protocolVer); + for (i = 0; i < def->source.nmountOpts; i++) virBufferAsprintf(&buf, "%s,", def->source.mountOpts[i]); diff --git a/tests/storagepoolxml2argvdata/pool-netfs-protocol-ver.argv b/tests/storagepoolxml2argvdata/pool-netfs-protocol-ver.argv new file mode 100644 index 0000000000..f26656d5b8 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-netfs-protocol-ver.argv @@ -0,0 +1 @@ +mount -t nfs localhost:/var/lib/libvirt/images /mnt -o nfsvers=3 diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index 0df97fb390..e8bae9fb8c 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -157,6 +157,7 @@ mymain(void) DO_TEST_FAIL("pool-iscsi-auth"); DO_TEST("pool-netfs"); DO_TEST("pool-netfs-auto"); + DO_TEST("pool-netfs-protocol-ver"); DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); DO_TEST("pool-netfs-mountopts"); -- 2.20.1
Allow the addition of the <protocol ver='n'/> to the provided XML. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-pool.c | 13 +++++++++++-- tools/virsh.pod | 6 +++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 79b33a8885..581fd06380 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -140,6 +140,10 @@ {.name = "source-mount-opts", \ .type = VSH_OT_STRING, \ .help = N_("comma separated list for NFS pool mount options") \ + }, \ + {.name = "source-protocol-ver", \ + .type = VSH_OT_STRING, \ + .help = N_("nfsvers value for NFS pool mount option") \ } virStoragePoolPtr @@ -323,7 +327,8 @@ virshBuildPoolXML(vshControl *ctl, *secretUsage = NULL, *adapterName = NULL, *adapterParent = NULL, *adapterWwnn = NULL, *adapterWwpn = NULL, *secretUUID = NULL, *adapterParentWwnn = NULL, *adapterParentWwpn = NULL, - *adapterParentFabricWwn = NULL, *mountOpts = NULL; + *adapterParentFabricWwn = NULL, *mountOpts = NULL, + *protoVer = NULL; size_t noptsList = 0; char **optsList = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -352,7 +357,8 @@ virshBuildPoolXML(vshControl *ctl, vshCommandOptStringReq(ctl, cmd, "adapter-parent-wwnn", &adapterParentWwnn) < 0 || vshCommandOptStringReq(ctl, cmd, "adapter-parent-wwpn", &adapterParentWwpn) < 0 || vshCommandOptStringReq(ctl, cmd, "adapter-parent-fabric-wwn", &adapterParentFabricWwn) < 0 || - vshCommandOptStringReq(ctl, cmd, "source-mount-opts", &mountOpts) < 0) + vshCommandOptStringReq(ctl, cmd, "source-mount-opts", &mountOpts) < 0 || + vshCommandOptStringReq(ctl, cmd, "source-protocol-ver", &protoVer) < 0) goto cleanup; if (mountOpts && @@ -406,6 +412,9 @@ virshBuildPoolXML(vshControl *ctl, if (srcName) virBufferAsprintf(&buf, "<name>%s</name>\n", srcName); + if (protoVer) + virBufferAsprintf(&buf, "<protocol ver='%s'/>\n", protoVer); + if (mountOpts) { size_t i; diff --git a/tools/virsh.pod b/tools/virsh.pod index 718efcb07e..95e01a0aa5 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3887,7 +3887,7 @@ just I<--build> is provided, then B<pool-build> is called with no flags. [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>]] -[I<--source-mount-opts mountOpts>] +[I<--source-protocol-ver ver>] [I<--source-mount-opts mountOpts>] [[I<--adapter-name name>] | [I<--adapter-wwnn> wwnn I<--adapter-wwpn> wwpn] [I<--adapter-parent parent> | I<--adapter-parent-wwnn parent_wwnn> I<adapter-parent-wwpn parent_wwpn> | @@ -3930,6 +3930,10 @@ the storage pool. The I<authtype> is either chap for iscsi I<type> pools or ceph for rbd I<type> pools. Either the secret I<usage> or I<uuid> value may be provided, but not both. +[I<--source-protocol-ver ver>] provides the NFS protocol version number used +to contact the server's NFS service via nfs mount option 'nfsvers=n'. It is +expect the I<ver> value is an unsigned integer. + [I<--source-mount-opts mountOpts>] provides a string to be used when creating the mount command for a netfs type pool. The I<mountOpts> must be in a comma separated list format. Valid I<mountOpts> that may be provided are "nosuid", -- 2.20.1
Introduce the infrastructure necessary to manage a Storage Pool XML Namespace. The general concept is similar to virDomainXMLNamespace, except that for Storage Pools the storage backend specific details can be stored within the _virStoragePoolOptions unlike the domain processing code which manages its xmlopt's via the virDomainXMLOption which is allocated/passed around for each domain. This patch defines the add the parse, format, free, and href methods required to process the XML and callout from the Storage Pool Def parse, format, and free API's to perform the action on the XML data for/from the backend. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 51 +++++++++++++++++++++++++++++++++++++++- src/conf/storage_conf.h | 24 +++++++++++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 8ec360e4a1..a20596e3f1 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -128,6 +128,9 @@ typedef virStoragePoolOptions *virStoragePoolOptionsPtr; struct _virStoragePoolOptions { unsigned int flags; int defaultFormat; + + virStoragePoolXMLNamespace ns; + virStoragePoolFormatToString formatToString; virStoragePoolFormatFromString formatFromString; }; @@ -322,6 +325,34 @@ virStoragePoolOptionsForPoolType(int type) } +/* virStoragePoolOptionsPoolTypeSetXMLNamespace: + * @type: virStoragePoolType + * @ns: xmlopt namespace pointer + * + * Store the @ns in the pool options for the particular backend. + * This allows the parse/format code to then directly call the Namespace + * method space (parse, format, href, free) as needed during processing. + * + * Returns: 0 on success, -1 on failure. + */ +int +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, + virStoragePoolXMLNamespacePtr ns) +{ + int ret = -1; + virStoragePoolTypeInfoPtr backend = virStoragePoolTypeInfoLookup(type); + + if (!backend) + goto cleanup; + + backend->poolOptions.ns = *ns; + ret = 0; + + cleanup: + return ret; +} + + static virStorageVolOptionsPtr virStorageVolOptionsForPoolType(int type) { @@ -409,6 +440,8 @@ virStoragePoolDefFree(virStoragePoolDefPtr def) VIR_FREE(def->target.path); VIR_FREE(def->target.perms.label); + if (def->namespaceData && def->ns.free) + (def->ns.free)(def->namespaceData); VIR_FREE(def); } @@ -870,6 +903,13 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) goto error; } + /* Make a copy of all the callback pointers here for easier use, + * especially during the virStoragePoolSourceClear method */ + ret->ns = options->ns; + if (ret->ns.parse && + (ret->ns.parse)(ctxt, &ret->namespaceData) < 0) + goto error; + cleanup: VIR_FREE(uuid); VIR_FREE(type); @@ -1058,7 +1098,10 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, _("unexpected pool type")); return -1; } - virBufferAsprintf(buf, "<pool type='%s'>\n", type); + virBufferAsprintf(buf, "<pool type='%s'", type); + if (def->namespaceData && def->ns.href) + virBufferAsprintf(buf, " %s", (def->ns.href)()); + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<name>%s</name>\n", def->name); @@ -1111,6 +1154,12 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</target>\n"); } + + if (def->namespaceData && def->ns.format) { + if ((def->ns.format)(buf, def->namespaceData) < 0) + return -1; + } + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</pool>\n"); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index f36d4daa3c..09930991aa 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -33,6 +33,26 @@ # include <libxml/tree.h> +/* Various callbacks needed to parse/create Storage Pool XML's using + * a private namespace */ +typedef int (*virStoragePoolDefNamespaceParse)(xmlXPathContextPtr, void **); +typedef void (*virStoragePoolDefNamespaceFree)(void *); +typedef int (*virStoragePoolDefNamespaceXMLFormat)(virBufferPtr, void *); +typedef const char *(*virStoragePoolDefNamespaceHref)(void); + +typedef struct _virStoragePoolXMLNamespace virStoragePoolXMLNamespace; +typedef virStoragePoolXMLNamespace *virStoragePoolXMLNamespacePtr; +struct _virStoragePoolXMLNamespace { + virStoragePoolDefNamespaceParse parse; + virStoragePoolDefNamespaceFree free; + virStoragePoolDefNamespaceXMLFormat format; + virStoragePoolDefNamespaceHref href; +}; + +int +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, + virStoragePoolXMLNamespacePtr ns); + /* * How the volume's data is stored on underlying * physical devices - can potentially span many @@ -226,6 +246,10 @@ struct _virStoragePoolDef { virStoragePoolSource source; virStoragePoolTarget target; + + /* Pool backend specific XML namespace data */ + void *namespaceData; + virStoragePoolXMLNamespace ns; }; typedef struct _virStoragePoolSourceList virStoragePoolSourceList; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c3d6306809..4d82797ee1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -919,6 +919,7 @@ virStoragePoolFormatDiskTypeToString; virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; virStoragePoolFormatLogicalTypeToString; +virStoragePoolOptionsPoolTypeSetXMLNamespace; virStoragePoolSaveConfig; virStoragePoolSaveState; virStoragePoolSourceClear; -- 2.20.1
Introduce the virStoragePoolNetFSMountOptionsDef to be used to manage the NFS Storage Pool XML Namespace for mount options. Using a new virStorageBackendNamespaceInit function, set the virStoragePoolXMLNamespace into the _virStoragePoolOptions when the storage backend is loaded. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 128 +++++++++++++++++++++++++++++++ src/storage/storage_util.c | 16 ++++ src/storage/storage_util.h | 14 ++++ 3 files changed, 158 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index dc9869417e..b82215fcdb 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -41,6 +41,7 @@ VIR_LOG_INIT("storage.storage_backend_fs"); #if WITH_STORAGE_FS +# include <libxml/xpathInternals.h> # include <mntent.h> struct _virNetfsDiscoverState { @@ -559,6 +560,122 @@ virStorageBackendFileSystemBuild(virStoragePoolObjPtr pool, } +#if WITH_STORAGE_FS + +# define STORAGE_POOL_NETFS_NAMESPACE_HREF "http://libvirt.org/schemas/storagepool/source/netfs/1.0" + +/* NetFS backend XML Namespace handling for nfs specific mount options to + * be added to the mount -o {options_list} command line. The XML will use + * the format, such as: + * + * <netfs:mount_opts> + * <netfs:option name='nodev'/> + * <netfs:option name='nosuid'/> + * </netfs:mount_opts> + * + * and the <pool type='netfs'> is required to have a "xmlns:netfs='%s'" + * attribute using the STORAGE_POOL_NETFS_NAMESPACE_HREF + */ + +static void +virStoragePoolDefNetFSNamespaceFree(void *nsdata) +{ + virStoragePoolNetFSMountOptionsDefPtr cmdopts = nsdata; + size_t i; + + if (!cmdopts) + return; + + for (i = 0; i < cmdopts->noptions; i++) + VIR_FREE(cmdopts->options[i]); + VIR_FREE(cmdopts->options); + + VIR_FREE(cmdopts); +} + + +static int +virStoragePoolDefNetFSNamespaceParse(xmlXPathContextPtr ctxt, + void **data) +{ + virStoragePoolNetFSMountOptionsDefPtr cmdopts = NULL; + xmlNodePtr *nodes = NULL; + int nnodes; + size_t i; + int ret = -1; + + if (xmlXPathRegisterNs(ctxt, BAD_CAST "netfs", + BAD_CAST STORAGE_POOL_NETFS_NAMESPACE_HREF) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register xml namespace '%s'"), + STORAGE_POOL_NETFS_NAMESPACE_HREF); + return -1; + } + + nnodes = virXPathNodeSet("./netfs:mount_opts/netfs:option", ctxt, &nodes); + if (nnodes < 0) + return -1; + + if (nnodes == 0) + return 0; + + if (VIR_ALLOC(cmdopts) < 0 || + VIR_ALLOC_N(cmdopts->options, nnodes) < 0) + goto cleanup; + + for (i = 0; i < nnodes; i++) { + if (!(cmdopts->options[cmdopts->noptions] = + virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no netfs mount option name specified")); + goto cleanup; + } + cmdopts->noptions++; + } + + VIR_STEAL_PTR(*data, cmdopts); + ret = 0; + + cleanup: + VIR_FREE(nodes); + virStoragePoolDefNetFSNamespaceFree(cmdopts); + return ret; +} + + +static int +virStoragePoolDefNetFSNamespaceFormatXML(virBufferPtr buf, + void *nsdata) +{ + size_t i; + virStoragePoolNetFSMountOptionsDefPtr def = nsdata; + + if (!def) + return 0; + + virBufferAddLit(buf, "<netfs:mount_opts>\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < def->noptions; i++) + virBufferEscapeString(buf, "<netfs:option name='%s'/>\n", + def->options[i]); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</netfs:mount_opts>\n"); + + return 0; +} + + +static const char * +virStoragePoolDefNetFSNamespaceHref(void) +{ + return "xmlns:netfs='" STORAGE_POOL_NETFS_NAMESPACE_HREF "'"; +} + +#endif /* WITH_STORAGE_FS */ + + virStorageBackend virStorageBackendDirectory = { .type = VIR_STORAGE_POOL_DIR, @@ -617,6 +734,13 @@ virStorageBackend virStorageBackendNetFileSystem = { .downloadVol = virStorageBackendVolDownloadLocal, .wipeVol = virStorageBackendVolWipeLocal, }; + +static virStoragePoolXMLNamespace virStoragePoolNetFSXMLNamespace = { + .parse = virStoragePoolDefNetFSNamespaceParse, + .free = virStoragePoolDefNetFSNamespaceFree, + .format = virStoragePoolDefNetFSNamespaceFormatXML, + .href = virStoragePoolDefNetFSNamespaceHref, +}; #endif /* WITH_STORAGE_FS */ @@ -632,6 +756,10 @@ virStorageBackendFsRegister(void) if (virStorageBackendRegister(&virStorageBackendNetFileSystem) < 0) return -1; + + if (virStorageBackendNamespaceInit(VIR_STORAGE_POOL_NETFS, + &virStoragePoolNetFSXMLNamespace) < 0) + return -1; #endif /* WITH_STORAGE_FS */ return 0; diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 0c52b797fc..1c0a8ee5cf 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -77,6 +77,22 @@ VIR_LOG_INIT("storage.storage_util"); + +/* virStorageBackendNamespaceInit: + * @poolType: virStoragePoolType + * @xmlns: Storage Pool specific namespace callback methods + * + * To be called during storage backend registration to configure the + * Storage Pool XML Namespace based on the backend's needs. + */ +int +virStorageBackendNamespaceInit(int poolType, + virStoragePoolXMLNamespacePtr xmlns) +{ + return virStoragePoolOptionsPoolTypeSetXMLNamespace(poolType, xmlns); +} + + #define READ_BLOCK_SIZE_DEFAULT (1024 * 1024) #define WRITE_BLOCK_SIZE_DEFAULT (4 * 1024) diff --git a/src/storage/storage_util.h b/src/storage/storage_util.h index c872468135..b11b1b23f7 100644 --- a/src/storage/storage_util.h +++ b/src/storage/storage_util.h @@ -26,6 +26,20 @@ # include "storage_driver.h" # include "storage_backend.h" +/* NetFS Storage Pool Namespace options to share w/ storage_backend_fs.c and + * the virStorageBackendFileSystemMountCmd method */ +typedef struct _virStoragePoolNetFSMountOptionsDef virStoragePoolNetFSMountOptionsDef; +typedef virStoragePoolNetFSMountOptionsDef *virStoragePoolNetFSMountOptionsDefPtr; +struct _virStoragePoolNetFSMountOptionsDef { + size_t noptions; + char **options; +}; + +int +virStorageBackendNamespaceInit(int poolType, + virStoragePoolXMLNamespacePtr xmlns); + + /* File creation/cloning functions used for cloning between backends */ int -- 2.20.1
Modify the storagepool.rng to allow for the usage of a different XML namespace to parse the netfs_mount_opts to be included with the netfs storage pool definition. Modify the storagepoolxml2xmltest to utilize a properly modified XML file to parse and format the namespace for a netfs storage pool. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 59 +++++++++++++++++++ docs/schemas/storagepool.rng | 20 +++++++ tests/Makefile.am | 4 +- .../pool-netfs-ns-mountopts.xml | 29 +++++++++ .../pool-netfs-ns-mountopts.xml | 29 +++++++++ tests/storagepoolxml2xmltest.c | 6 ++ 6 files changed, 146 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-ns-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-ns-mountopts.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 58f5eb2e2e..525e82552c 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -509,6 +509,65 @@ device, measured in bytes. <span class="since">Since 0.4.1</span> </p> + <h3><a id="StoragePoolNamespaces">Storage Pool Namespaces</a></h3> + + <p> + Usage of Storage Pool Namespaces provides a mechanism to provide + pool type specific data in a free form or abitrary manner via + XML syntax targeted solely for the needs of the specific pool type + which is not otherwise supportable via XML. For the "netfs" pool + this provides a mechanism to provide additional mount options on the + command line not listed/supported for the <code>mount_opts</code> + element. + </p> + <dl> + <dt><code>netfs:mount_opts</code></dt> + <dd>Provides an XML namespace mechanism to optionally utilize + specifically named options for the mount command via the "-o" + option for the <code>netfs</code> type storage pools. That are + not provided by the <code>mount_opts</code> element. In order + to designate that the Storage Pool will be using the mechanism, + the <code>pool</code> element must be modified to provide the + XML namespace attribute syntax as follows: + + <p> + xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0' + </p> + + <p> + The <code>netfs:mount_opts</code> defines the mount options by + specifying multiple <code>netfs:option</code> subelements with + the attribute <code>name</code> specifying the mount option to + be added. The value of the named option is not checked since + it's possible options don't exist on all distributions. It is + expected that proper and valid options will be supplied for the + target host. + </p> + + The following XML snippet shows the syntax required in order to + utilize + <pre> +<pool type="netfs" xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0'>; + <name>nfsimages</name> +... + <source> +... + </source> +... + <target> +... + </target> + <netfs:mount_opts> + <netfs:option name='sync'/> + <netfs:option name='lazytime'/> + </netfs:mount_opts> +</pool> +...</pre> + + <span class="since">Since 5.1.0.</span></dd> + + </dl> + <h2><a id="StorageVol">Storage volume XML</a></h2> <p> A storage volume will generally be either a file or a device diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index a32c89b94c..200bd07539 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -64,6 +64,9 @@ <ref name='sourcenetfs'/> <ref name='target'/> </interleave> + <optional> + <ref name='netfs_mount_opts'/> + </optional> </define> <define name='poollogical'> @@ -702,4 +705,21 @@ </data> </define> + <!-- + Optional storage pool extensions in their own namespace: + NetFS + --> + + <define name="netfs_mount_opts"> + <element name="mount_opts" ns="http://libvirt.org/schemas/storagepool/source/netfs/1.0"> + <zeroOrMore> + <element name="option"> + <attribute name='name'> + <text/> + </attribute> + </element> + </zeroOrMore> + </element> + </define> + </grammar> diff --git a/tests/Makefile.am b/tests/Makefile.am index f74d8463b6..ab4c716529 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -937,7 +937,9 @@ storagevolxml2xmltest_LDADD = $(LDADDS) storagepoolxml2xmltest_SOURCES = \ storagepoolxml2xmltest.c \ testutils.c testutils.h -storagepoolxml2xmltest_LDADD = $(LDADDS) +storagepoolxml2xmltest_LDADD = $(LDADDS) \ + ../src/libvirt_driver_storage_impl.la \ + $(GNULIB_LIBS) nodedevxml2xmltest_SOURCES = \ nodedevxml2xmltest.c \ diff --git a/tests/storagepoolxml2xmlin/pool-netfs-ns-mountopts.xml b/tests/storagepoolxml2xmlin/pool-netfs-ns-mountopts.xml new file mode 100644 index 0000000000..68c51da399 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-netfs-ns-mountopts.xml @@ -0,0 +1,29 @@ +<pool type='netfs' xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0'> + <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'/> + <format type='nfs'/> + <protocol ver='3'/> + <mount_opts> + <option name='nodev'/> + <option name='nosuid'/> + </mount_opts> + </source> + <target> + <path>/mnt</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> + <netfs:mount_opts> + <netfs:option name='sync'/> + <netfs:option name='lazytime'/> + </netfs:mount_opts> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-netfs-ns-mountopts.xml b/tests/storagepoolxml2xmlout/pool-netfs-ns-mountopts.xml new file mode 100644 index 0000000000..82df12d646 --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-netfs-ns-mountopts.xml @@ -0,0 +1,29 @@ +<pool type='netfs' xmlns:netfs='http://libvirt.org/schemas/storagepool/source/netfs/1.0'> + <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='nfs'/> + <protocol ver='3'/> + <mount_opts> + <option name='nodev'/> + <option name='nosuid'/> + </mount_opts> + </source> + <target> + <path>/mnt</path> + <permissions> + <mode>0700</mode> + <owner>0</owner> + <group>0</group> + </permissions> + </target> + <netfs:mount_opts> + <netfs:option name='sync'/> + <netfs:option name='lazytime'/> + </netfs:mount_opts> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index ecdc2715b0..6b6653dcfe 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -11,6 +11,8 @@ #include "testutilsqemu.h" #include "virstring.h" +#include "storage/storage_util.h" + #define VIR_FROM_THIS VIR_FROM_NONE static int @@ -70,6 +72,9 @@ mymain(void) testCompareXMLToXMLHelper, (name)) < 0) \ ret = -1 + if (storageRegisterAll() < 0) + return EXIT_FAILURE; + DO_TEST("pool-dir"); DO_TEST("pool-dir-naming"); DO_TEST("pool-fs"); @@ -87,6 +92,7 @@ mymain(void) DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); DO_TEST("pool-netfs-mountopts"); + DO_TEST("pool-netfs-ns-mountopts"); DO_TEST("pool-scsi"); DO_TEST("pool-scsi-type-scsi-host"); DO_TEST("pool-scsi-type-fc-host"); -- 2.20.1
If the NetFS Storage Pool Namespace XML data exists, format the mount options on the MOUNT command line. When the pool is started, the options will be generated on the command line along with the options already defined. To view the options of the running pool, use either 'nfsstat -m' or 'grep $POOLNAME /proc/mounts' for the added Flags/options. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 10 +++++++++- .../pool-netfs-ns-mountopts.argv | 2 ++ tests/storagepoolxml2argvtest.c | 4 ++++ 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-ns-mountopts.argv diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 1c0a8ee5cf..8edcd23e2f 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -4354,7 +4354,8 @@ virStorageBackendFileSystemMountCmd(const char *cmdstr, virStorageBackendFileSystemMountDefaultArgs(cmd, src, def); if (def->type == VIR_STORAGE_POOL_NETFS && - (def->source.mountOpts || (def->source.protocolVer > 0))) { + (def->source.mountOpts || (def->source.protocolVer > 0) || + def->namespaceData)) { size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; VIR_AUTOFREE(char *) mountOpts = NULL; @@ -4365,6 +4366,13 @@ virStorageBackendFileSystemMountCmd(const char *cmdstr, for (i = 0; i < def->source.nmountOpts; i++) virBufferAsprintf(&buf, "%s,", def->source.mountOpts[i]); + if (def->namespaceData) { + virStoragePoolNetFSMountOptionsDefPtr opts = def->namespaceData; + + for (i = 0; i < opts->noptions; i++) + virBufferAsprintf(&buf, "%s,", opts->options[i]); + } + virBufferTrim(&buf, ",", -1); if (virBufferCheckError(&buf) < 0) diff --git a/tests/storagepoolxml2argvdata/pool-netfs-ns-mountopts.argv b/tests/storagepoolxml2argvdata/pool-netfs-ns-mountopts.argv new file mode 100644 index 0000000000..87c5c83c47 --- /dev/null +++ b/tests/storagepoolxml2argvdata/pool-netfs-ns-mountopts.argv @@ -0,0 +1,2 @@ +mount -t nfs localhost:/var/lib/libvirt/images /mnt -o nfsvers=3,nodev,nosuid,\ +sync,lazytime diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c index e8bae9fb8c..84a9075c78 100644 --- a/tests/storagepoolxml2argvtest.c +++ b/tests/storagepoolxml2argvtest.c @@ -144,6 +144,9 @@ mymain(void) #define DO_TEST_FAIL(pool, ...) \ DO_TEST_FULL(true, pool) + if (storageRegisterAll() < 0) + return EXIT_FAILURE; + DO_TEST_FAIL("pool-dir"); DO_TEST_FAIL("pool-dir-naming"); DO_TEST("pool-fs"); @@ -161,6 +164,7 @@ mymain(void) DO_TEST("pool-netfs-gluster"); DO_TEST("pool-netfs-cifs"); DO_TEST("pool-netfs-mountopts"); + DO_TEST("pool-netfs-ns-mountopts"); DO_TEST_FAIL("pool-scsi"); DO_TEST_FAIL("pool-scsi-type-scsi-host"); DO_TEST_FAIL("pool-scsi-type-fc-host"); -- 2.20.1
Allow for adjustment of RBD configuration options via Storage Pool XML Namespace adjustments. Based off original patch/concept: https://www.redhat.com/archives/libvir-list/2014-May/msg00940.html Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 52 +++++- docs/schemas/storagepool.rng | 23 +++ src/storage/storage_backend_rbd.c | 166 +++++++++++++++++- .../pool-rbd-ns-configopts.xml | 17 ++ .../pool-rbd-ns-configopts.xml | 20 +++ tests/storagepoolxml2xmltest.c | 1 + 6 files changed, 275 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-ns-configopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-ns-configopts.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 525e82552c..d34c6f2cc0 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -518,7 +518,8 @@ which is not otherwise supportable via XML. For the "netfs" pool this provides a mechanism to provide additional mount options on the command line not listed/supported for the <code>mount_opts</code> - element. + element. For the "rbd" pool this provides a mechanism to override + default settings for RBD configuration options. </p> <dl> <dt><code>netfs:mount_opts</code></dt> @@ -566,6 +567,55 @@ <span class="since">Since 5.1.0.</span></dd> + <dt><code>rbd:config_opts</code></dt> + <dd>Provides an XML namespace mechanism to optionally utilize + specifically named options for the RBD configuration options + via the rados_conf_set API for the <code>rbd</code> type + storage pools. In order to designate that the Storage Pool + will be using the mechanism, the <code>pool</code> element + must be modified to provide the XML namespace attribute + syntax as follows: + + <p> + xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0' + </p> + + <p> + The <code>rbd:config_opts</code> defines the configuration options + by specifying multiple <code>rbd:option</code> subelements with + the attribute <code>name</code> specifying the configuration option + to be added and <code>value</code> specifying the configuration + option value. The name and value for each option is only checked + to be not empty. The name and value provided are not checked since + it's possible options don't exist on all distributions. It is + expected that proper and valid options will be supplied for the + target host. + </p> + + The following XML snippet shows the syntax required in order to + utilize + <pre> +<pool type="rbd" xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0'>; + <name>myrbdpool</name> +... + <source> +... + </source> +... + <target> +... + </target> +... + <rbd:config_opts> + <rbd:option name='client_mount_timeout' value='45'/> + <rbd:option name='rados_mon_op_timeout' value='20'/> + <rbd:option name='rados_osd_op_timeout' value='10'/> + </rbd:config_opts> +</pool> +</pre> + + <span class="since">Since 5.1.0.</span></dd> + </dl> <h2><a id="StorageVol">Storage volume XML</a></h2> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 200bd07539..67efbbc9cf 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -153,6 +153,9 @@ <ref name='sizing'/> <ref name='sourcerbd'/> </interleave> + <optional> + <ref name='rbd_config_opts'/> + </optional> </define> <define name='poolsheepdog'> @@ -722,4 +725,24 @@ </element> </define> + <!-- + Optional storage pool extensions in their own namespace: + RBD + --> + + <define name="rbd_config_opts"> + <element name="config_opts" ns="http://libvirt.org/schemas/storagepool/source/rbd/1.0"> + <zeroOrMore> + <element name="option"> + <attribute name='name'> + <text/> + </attribute> + <attribute name='value'> + <text/> + </attribute> + </element> + </zeroOrMore> + </element> + </define> + </grammar> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 24dd1349ae..cc38b76ff2 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -36,6 +36,7 @@ #include "rbd/librbd.h" #include "secret_util.h" #include "storage_util.h" +#include <libxml/xpathInternals.h> #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -50,6 +51,140 @@ struct _virStorageBackendRBDState { typedef struct _virStorageBackendRBDState virStorageBackendRBDState; typedef virStorageBackendRBDState *virStorageBackendRBDStatePtr; +/* NetFS Storage Pool Namespace options to share w/ storage_backend_fs.c and + * the virStorageBackendFileSystemMountCmd method */ +typedef struct _virStoragePoolRBDMountOptionsDef virStoragePoolRBDMountOptionsDef; +typedef virStoragePoolRBDMountOptionsDef *virStoragePoolRBDMountOptionsDefPtr; +struct _virStoragePoolRBDMountOptionsDef { + size_t noptions; + char **names; + char **values; +}; + +#define STORAGE_POOL_RBD_NAMESPACE_HREF "http://libvirt.org/schemas/storagepool/source/rbd/1.0" + +static void +virStoragePoolDefRBDNamespaceFree(void *nsdata) +{ + virStoragePoolRBDMountOptionsDefPtr cmdopts = nsdata; + size_t i; + + if (!cmdopts) + return; + + for (i = 0; i < cmdopts->noptions; i++) { + VIR_FREE(cmdopts->names[i]); + VIR_FREE(cmdopts->values[i]); + } + VIR_FREE(cmdopts->names); + VIR_FREE(cmdopts->values); + + VIR_FREE(cmdopts); +} + + +static int +virStoragePoolDefRBDNamespaceParse(xmlXPathContextPtr ctxt, + void **data) +{ + virStoragePoolRBDMountOptionsDefPtr cmdopts = NULL; + xmlNodePtr *nodes = NULL; + int nnodes; + size_t i; + int ret = -1; + + if (xmlXPathRegisterNs(ctxt, BAD_CAST "rbd", + BAD_CAST STORAGE_POOL_RBD_NAMESPACE_HREF) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register xml namespace '%s'"), + STORAGE_POOL_RBD_NAMESPACE_HREF); + return -1; + } + + nnodes = virXPathNodeSet("./rbd:config_opts/rbd:option", ctxt, &nodes); + if (nnodes < 0) + return -1; + + if (nnodes == 0) + return 0; + + if (VIR_ALLOC(cmdopts) < 0) + goto cleanup; + + if (VIR_ALLOC_N(cmdopts->names, nnodes) < 0 || + VIR_ALLOC_N(cmdopts->values, nnodes) < 0) + goto cleanup; + + for (i = 0; i < nnodes; i++) { + if (!(cmdopts->names[cmdopts->noptions] = + virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no rbd option name specified")); + goto cleanup; + } + if (*cmdopts->names[cmdopts->noptions] == '\0') { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("empty rbd option name specified")); + goto cleanup; + } + if (!(cmdopts->values[cmdopts->noptions] = + virXMLPropString(nodes[i], "value"))) { + virReportError(VIR_ERR_XML_ERROR, + _("no rbd option value specified for name '%s'"), + cmdopts->names[cmdopts->noptions]); + goto cleanup; + } + if (*cmdopts->values[cmdopts->noptions] == '\0') { + virReportError(VIR_ERR_XML_ERROR, + _("empty rbd option value specified for name '%s'"), + cmdopts->names[cmdopts->noptions]); + goto cleanup; + } + cmdopts->noptions++; + } + + VIR_STEAL_PTR(*data, cmdopts); + ret = 0; + + cleanup: + VIR_FREE(nodes); + virStoragePoolDefRBDNamespaceFree(cmdopts); + return ret; +} + + +static int +virStoragePoolDefRBDNamespaceFormatXML(virBufferPtr buf, + void *nsdata) +{ + size_t i; + virStoragePoolRBDMountOptionsDefPtr def = nsdata; + + if (!def) + return 0; + + virBufferAddLit(buf, "<rbd:config_opts>\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < def->noptions; i++) { + virBufferEscapeString(buf, "<rbd:option name='%s' ", def->names[i]); + virBufferEscapeString(buf, "value='%s'/>\n", def->values[i]); + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</rbd:config_opts>\n"); + + return 0; +} + + +static const char * +virStoragePoolDefRBDNamespaceHref(void) +{ + return "xmlns:rbd='" STORAGE_POOL_RBD_NAMESPACE_HREF "'"; +} + + static int virStorageBackendRBDRADOSConfSet(rados_t cluster, const char *option, @@ -69,10 +204,11 @@ virStorageBackendRBDRADOSConfSet(rados_t cluster, static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, - virStoragePoolSourcePtr source) + virStoragePoolDefPtr def) { int ret = -1; int r = 0; + virStoragePoolSourcePtr source = &def->source; virStorageAuthDefPtr authdef = source->auth; unsigned char *secret_value = NULL; size_t secret_value_size = 0; @@ -183,6 +319,17 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, rbd_default_format) < 0) goto cleanup; + if (def->namespaceData) { + virStoragePoolRBDMountOptionsDefPtr cmdopts = def->namespaceData; + + for (i = 0; i < cmdopts->noptions; i++) { + if (virStorageBackendRBDRADOSConfSet(ptr->cluster, + cmdopts->names[i], + cmdopts->values[i]) < 0) + goto cleanup; + } + } + ptr->starttime = time(0); if ((r = rados_connect(ptr->cluster)) < 0) { virReportSystemError(-r, _("failed to connect to the RADOS monitor on: %s"), @@ -256,7 +403,7 @@ virStorageBackendRBDNewState(virStoragePoolObjPtr pool) if (VIR_ALLOC(ptr) < 0) return NULL; - if (virStorageBackendRBDOpenRADOSConn(ptr, &def->source) < 0) + if (virStorageBackendRBDOpenRADOSConn(ptr, def) < 0) goto error; if (virStorageBackendRBDOpenIoCTX(ptr, pool) < 0) @@ -1277,6 +1424,7 @@ virStorageBackendRBDVolWipe(virStoragePoolObjPtr pool, return ret; } + virStorageBackend virStorageBackendRBD = { .type = VIR_STORAGE_POOL_RBD, @@ -1291,8 +1439,20 @@ virStorageBackend virStorageBackendRBD = { }; +static virStoragePoolXMLNamespace virStoragePoolRBDXMLNamespace = { + .parse = virStoragePoolDefRBDNamespaceParse, + .free = virStoragePoolDefRBDNamespaceFree, + .format = virStoragePoolDefRBDNamespaceFormatXML, + .href = virStoragePoolDefRBDNamespaceHref, +}; + + int virStorageBackendRBDRegister(void) { - return virStorageBackendRegister(&virStorageBackendRBD); + if (virStorageBackendRegister(&virStorageBackendRBD) < 0) + return -1; + + return virStorageBackendNamespaceInit(VIR_STORAGE_POOL_RBD, + &virStoragePoolRBDXMLNamespace); } diff --git a/tests/storagepoolxml2xmlin/pool-rbd-ns-configopts.xml b/tests/storagepoolxml2xmlin/pool-rbd-ns-configopts.xml new file mode 100644 index 0000000000..6b62aa6f7e --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-rbd-ns-configopts.xml @@ -0,0 +1,17 @@ +<pool type='rbd' xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0'> + <name>ceph</name> + <uuid>47c1faee-0207-e741-f5ae-d9b019b98fe2</uuid> + <source> + <name>rbd</name> + <host name='localhost' port='6789'/> + <host name='localhost' port='6790'/> + <auth username='admin' type='ceph'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + </source> + <rbd:config_opts> + <rbd:option name='client_mount_timeout' value='45'/> + <rbd:option name='rados_mon_op_timeout' value='10'/> + <rbd:option name='rados_osd_op_timeout' value='20'/> + </rbd:config_opts> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-rbd-ns-configopts.xml b/tests/storagepoolxml2xmlout/pool-rbd-ns-configopts.xml new file mode 100644 index 0000000000..342a0ff74a --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-rbd-ns-configopts.xml @@ -0,0 +1,20 @@ +<pool type='rbd' xmlns:rbd='http://libvirt.org/schemas/storagepool/source/rbd/1.0'> + <name>ceph</name> + <uuid>47c1faee-0207-e741-f5ae-d9b019b98fe2</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='localhost' port='6789'/> + <host name='localhost' port='6790'/> + <name>rbd</name> + <auth type='ceph' username='admin'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + </source> + <rbd:config_opts> + <rbd:option name='client_mount_timeout' value='45'/> + <rbd:option name='rados_mon_op_timeout' value='10'/> + <rbd:option name='rados_osd_op_timeout' value='20'/> + </rbd:config_opts> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 6b6653dcfe..16cb032244 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -107,6 +107,7 @@ mymain(void) DO_TEST("pool-zfs"); DO_TEST("pool-zfs-sourcedev"); DO_TEST("pool-rbd"); + DO_TEST("pool-rbd-ns-configopts"); DO_TEST("pool-vstorage"); DO_TEST("pool-iscsi-direct-auth"); DO_TEST("pool-iscsi-direct"); -- 2.20.1
participants (3)
-
Cole Robinson -
Daniel P. Berrangé -
John Ferlan