Re: [libvirt] [PATCH v3 01/11] conf: Add some defined NFS Storage Pool mount options

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(a)redhat.com&gt; > --- > 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 @@ > &lt;/source&gt; > ...</pre> > > + <pre> > +... > + &lt;source&gt; > + &lt;host name='localhost'/&gt; > + &lt;dir path='/var/lib/libvirt/images'/&gt; > + &lt;format type='nfs'/&gt; > + &lt;mount_opts&gt; > + &lt;option name='nodev'/&gt; > + &lt;option name='nosuid'/&gt; > + &lt;/mount_opts&gt; > + &lt;/source&gt; > +...</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>