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

[Daniel P . Berrangé]

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 <jfer...@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 @@
>  &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>

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list