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