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

[Cole Robinson]

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