[libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command

John Ferlan posted 6 patches 5 years, 3 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190108175226.32693-1-jferlan@redhat.com
There is a newer version of this series
docs/formatstorage.html.in                    |  87 ++++++++++
docs/schemas/storagepool.rng                  |  43 +++++
src/conf/storage_conf.c                       |  51 +++++-
src/conf/storage_conf.h                       |  24 +++
src/libvirt_private.syms                      |   1 +
src/storage/storage_backend_fs.c              | 127 ++++++++++++++
src/storage/storage_backend_rbd.c             | 159 +++++++++++++++++-
src/storage/storage_util.c                    |  36 ++++
src/storage/storage_util.h                    |  14 ++
tests/Makefile.am                             |   4 +-
.../pool-netfs-mountopts.argv                 |   1 +
tests/storagepoolxml2argvtest.c               |   4 +
.../pool-netfs-mountopts.xml                  |  24 +++
.../pool-rbd-configopts.xml                   |  17 ++
.../pool-netfs-mountopts.xml                  |  24 +++
.../pool-rbd-configopts.xml                   |  20 +++
tests/storagepoolxml2xmltest.c                |   7 +
tools/virsh-pool.c                            |  39 ++++-
tools/virsh.pod                               |   5 +
19 files changed, 680 insertions(+), 7 deletions(-)
create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv
create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml
create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml
create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml
create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml
[libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by John Ferlan 5 years, 3 months ago
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html

Kept the subject the same, but the concept has been adjusted to follow
issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML.
This series adds both the NFS and the RBD adjustments that were essentially
in the referenced series from review.

Changes since v1:

 * Patch 1: (NEW) Implements a bare-bones infrastructure for storage pool
            XML namespace. Could be split again, but separately the patches
            were too small IMO.

 * Patch 2: (NEW) Implementation of providing XML Namespace functions. The
            details of the operation are in the subsequent patch. I could
            combine 2 and 3, but perhaps it's easier to review separated.

 * Patch 3: (REWORK) Rework the previous series patch1 in order to utilize
            the Storage Pool XML Namespace. Adjust the description of the
            configuration and modify the storagepoolxml2xmltest appropriately.

 * Patch 4: (REWORK) Utilize the XML Namespace data to generate the command
            line which doesn't change since v1.

 * Patch 5: (REWORK) Rework the virsh code in order to generate the proper
            syntax to utilize Storage Pool XML Namespaces.

 * Patch 6: (NEW) Well, new as in this series, but it's essentially a rework
            of what was posted in the series jtomko referenced. The code will
            "prove" (to a degree) that the concept can work for multiple pool
            types. The "downside" is that there's no (easy) way that I could
            find to generate output to show the commands are executed.

John Ferlan (6):
  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 mount options to command line
  virsh: Add source-mount-opts for pool commands
  rbd: Utilize storage pool namespace to manage config options

 docs/formatstorage.html.in                    |  87 ++++++++++
 docs/schemas/storagepool.rng                  |  43 +++++
 src/conf/storage_conf.c                       |  51 +++++-
 src/conf/storage_conf.h                       |  24 +++
 src/libvirt_private.syms                      |   1 +
 src/storage/storage_backend_fs.c              | 127 ++++++++++++++
 src/storage/storage_backend_rbd.c             | 159 +++++++++++++++++-
 src/storage/storage_util.c                    |  36 ++++
 src/storage/storage_util.h                    |  14 ++
 tests/Makefile.am                             |   4 +-
 .../pool-netfs-mountopts.argv                 |   1 +
 tests/storagepoolxml2argvtest.c               |   4 +
 .../pool-netfs-mountopts.xml                  |  24 +++
 .../pool-rbd-configopts.xml                   |  17 ++
 .../pool-netfs-mountopts.xml                  |  24 +++
 .../pool-rbd-configopts.xml                   |  20 +++
 tests/storagepoolxml2xmltest.c                |   7 +
 tools/virsh-pool.c                            |  39 ++++-
 tools/virsh.pod                               |   5 +
 19 files changed, 680 insertions(+), 7 deletions(-)
 create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv
 create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml
 create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml
 create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml
 create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml

-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by Michal Privoznik 5 years, 3 months ago
On 1/8/19 6:52 PM, John Ferlan wrote:
> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
> 
> Kept the subject the same, but the concept has been adjusted to follow
> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML.
> This series adds both the NFS and the RBD adjustments that were essentially
> in the referenced series from review.
> 
> Changes since v1:
> 
>  * Patch 1: (NEW) Implements a bare-bones infrastructure for storage pool
>             XML namespace. Could be split again, but separately the patches
>             were too small IMO.
> 
>  * Patch 2: (NEW) Implementation of providing XML Namespace functions. The
>             details of the operation are in the subsequent patch. I could
>             combine 2 and 3, but perhaps it's easier to review separated.
> 
>  * Patch 3: (REWORK) Rework the previous series patch1 in order to utilize
>             the Storage Pool XML Namespace. Adjust the description of the
>             configuration and modify the storagepoolxml2xmltest appropriately.
> 
>  * Patch 4: (REWORK) Utilize the XML Namespace data to generate the command
>             line which doesn't change since v1.
> 
>  * Patch 5: (REWORK) Rework the virsh code in order to generate the proper
>             syntax to utilize Storage Pool XML Namespaces.
> 
>  * Patch 6: (NEW) Well, new as in this series, but it's essentially a rework
>             of what was posted in the series jtomko referenced. The code will
>             "prove" (to a degree) that the concept can work for multiple pool
>             types. The "downside" is that there's no (easy) way that I could
>             find to generate output to show the commands are executed.
> 
> John Ferlan (6):
>   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 mount options to command line
>   virsh: Add source-mount-opts for pool commands
>   rbd: Utilize storage pool namespace to manage config options
> 
>  docs/formatstorage.html.in                    |  87 ++++++++++
>  docs/schemas/storagepool.rng                  |  43 +++++
>  src/conf/storage_conf.c                       |  51 +++++-
>  src/conf/storage_conf.h                       |  24 +++
>  src/libvirt_private.syms                      |   1 +
>  src/storage/storage_backend_fs.c              | 127 ++++++++++++++
>  src/storage/storage_backend_rbd.c             | 159 +++++++++++++++++-
>  src/storage/storage_util.c                    |  36 ++++
>  src/storage/storage_util.h                    |  14 ++
>  tests/Makefile.am                             |   4 +-
>  .../pool-netfs-mountopts.argv                 |   1 +
>  tests/storagepoolxml2argvtest.c               |   4 +
>  .../pool-netfs-mountopts.xml                  |  24 +++
>  .../pool-rbd-configopts.xml                   |  17 ++
>  .../pool-netfs-mountopts.xml                  |  24 +++
>  .../pool-rbd-configopts.xml                   |  20 +++
>  tests/storagepoolxml2xmltest.c                |   7 +
>  tools/virsh-pool.c                            |  39 ++++-
>  tools/virsh.pod                               |   5 +
>  19 files changed, 680 insertions(+), 7 deletions(-)
>  create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv
>  create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml
>  create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml
>  create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml
>  create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml
> 


ACK

My only worry is that we don't validate the options in any way. They are
basically a text free form which we will put onto 'mount' cmd line directly.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by John Ferlan 5 years, 3 months ago

On 1/9/19 11:55 AM, Michal Privoznik wrote:

[...]

> 
> 
> ACK
> 

Thanks for the review... although it seems I'll have to consider a 3rd
approach as a result of Daniel's comments. Nothing is ever easy.

> My only worry is that we don't validate the options in any way. They are
> basically a text free form which we will put onto 'mount' cmd line directly.
> 

I thought about making sure of specific chars for the attributes (there
are some examples in other .rng files), but I know as soon as I do,
there's some request for some char that wasn't included. Going with
<text/> was just the bail out especially since I find regex's to be
unintelligible to fully decipher.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by Daniel P. Berrangé 5 years, 3 months ago
On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
> 
> Kept the subject the same, but the concept has been adjusted to follow
> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML.
> This series adds both the NFS and the RBD adjustments that were essentially
> in the referenced series from review.

Can you give any info about what motivated this addition.

Anything that is implemented via a separate XML namespace is
generally considered

   "unsupported, you're on your own when it breaks"

so if this feature is neeeded for any management application like
oVirt, OpenStack, etc, then using a custom namespace is not going
to be a suitable approach.


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
Re: [libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by John Ferlan 5 years, 3 months ago

On 1/9/19 12:09 PM, Daniel P. Berrangé wrote:
> On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
>> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
>>
>> Kept the subject the same, but the concept has been adjusted to follow
>> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML.
>> This series adds both the NFS and the RBD adjustments that were essentially
>> in the referenced series from review.
> 
> Can you give any info about what motivated this addition.

There's a bz:

https://bugzilla.redhat.com/show_bug.cgi?id=1584663

which along the way has had private comments - I probably should add the
bz to at least one of the patches, but I think because of the (probably
unnecessary) private comments in the bz, I was hesitant to do so.

I tried a different mechanism and Jano pointed out:

https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html

that previous attempts were rejected due to the arbitrary text.

> 
> Anything that is implemented via a separate XML namespace is
> generally considered
> 
>    "unsupported, you're on your own when it breaks"
> 
> so if this feature is neeeded for any management application like
> oVirt, OpenStack, etc, then using a custom namespace is not going
> to be a suitable approach.

And I don't think it would be palatable to add N 'text' options to the
XML that map to the same N 'text' options in the mount command. So this
was the next best option as far as I saw it. That'd be one of those
never ending tasks to add the favorite option.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by Daniel P. Berrangé 5 years, 3 months ago
On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote:
> 
> 
> On 1/9/19 12:09 PM, Daniel P. Berrangé wrote:
> > On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
> >> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
> >>
> >> Kept the subject the same, but the concept has been adjusted to follow
> >> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML.
> >> This series adds both the NFS and the RBD adjustments that were essentially
> >> in the referenced series from review.
> > 
> > Can you give any info about what motivated this addition.
> 
> There's a bz:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1584663
> 
> which along the way has had private comments - I probably should add the
> bz to at least one of the patches, but I think because of the (probably
> unnecessary) private comments in the bz, I was hesitant to do so.
> 
> I tried a different mechanism and Jano pointed out:
> 
> https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html
> 
> that previous attempts were rejected due to the arbitrary text.

Ok, from the rather limited information in the BZ above, it is clear
that this feature is required to be production supported, so that
rules out use of private XML namespaces as a viable solution.

I agree with Jano's rejected of arbitrary mount option passthrough in
v1 too though.

I think the right answer involves multiple approaches in the XML.

For the NFS pool, we should have an explicit way to request the NFS
protocol version in the XML.

For the general nosuid, nodev flags, I think we can do something like we
have for the <features/> element in the domain XML.

Or on second thoughts, the only reason for the storage pools mounts is to
provide VM disk images, so we should just unconditionally always set nosuid
& nodev when running mount. No storage volume we report should be setuid or
be a device node.  If the NFS mount has a directory tree for container
filesystems, then the previously discussed  virFileSystem APIs should be
actually implemented.

We could none the less also still have a private XML namespace for
doing arbitrary mount argument passthrough, but that shouldn't be
the solution for this particular BZ. It is just a way to get a quick
workaround for an option while we decide how to model the desired
new mount option explicitly in the XML, as we do for QEMU options.

> 
> > 
> > Anything that is implemented via a separate XML namespace is
> > generally considered
> > 
> >    "unsupported, you're on your own when it breaks"
> > 
> > so if this feature is neeeded for any management application like
> > oVirt, OpenStack, etc, then using a custom namespace is not going
> > to be a suitable approach.
> 
> And I don't think it would be palatable to add N 'text' options to the
> XML that map to the same N 'text' options in the mount command. So this
> was the next best option as far as I saw it. That'd be one of those
> never ending tasks to add the favorite option.

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
Re: [libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by John Ferlan 5 years, 3 months ago

On 1/9/19 12:40 PM, Daniel P. Berrangé wrote:
> On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote:
>>
>>
>> On 1/9/19 12:09 PM, Daniel P. Berrangé wrote:
>>> On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
>>>> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
>>>>
>>>> Kept the subject the same, but the concept has been adjusted to follow
>>>> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML.
>>>> This series adds both the NFS and the RBD adjustments that were essentially
>>>> in the referenced series from review.
>>>
>>> Can you give any info about what motivated this addition.
>>
>> There's a bz:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1584663
>>
>> which along the way has had private comments - I probably should add the
>> bz to at least one of the patches, but I think because of the (probably
>> unnecessary) private comments in the bz, I was hesitant to do so.
>>
>> I tried a different mechanism and Jano pointed out:
>>
>> https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html
>>
>> that previous attempts were rejected due to the arbitrary text.
> 
> Ok, from the rather limited information in the BZ above, it is clear
> that this feature is required to be production supported, so that
> rules out use of private XML namespaces as a viable solution.
> 
> I agree with Jano's rejected of arbitrary mount option passthrough in
> v1 too though.

I understand the rejection and I also found namespaces to not be very
user or developer friendly. Even less easy to understand were those
"other' namespaces using "ns0" and/or "ns1". Proverbial rock and hard
place scenario.

It's no small wonder why the patches that were rejected in June 2014 had
no attempted followup that I found - it just didn't seem worth the
effort and using their own private code to suit their own needs was far
more simple!

> 
> I think the right answer involves multiple approaches in the XML.
> 
> For the NFS pool, we should have an explicit way to request the NFS
> protocol version in the XML.

You mean mount option "nfsvers=n"?

> 
> For the general nosuid, nodev flags, I think we can do something like we
> have for the <features/> element in the domain XML.
> 

So that means to me that every option currently possible in mount would
need to be listed.  Doesn't that feel excessive?  Not that mount changes
that often, but I would think it's awfully painful to take that route.
Especially for some of the "more complicated" mount options.

> Or on second thoughts, the only reason for the storage pools mounts is to
> provide VM disk images, so we should just unconditionally always set nosuid
> & nodev when running mount. No storage volume we report should be setuid or
> be a device node.  If the NFS mount has a directory tree for container
> filesystems, then the previously discussed  virFileSystem APIs should be
> actually implemented.

That'd be the really simple simple fix at least for nosuid and nodev. I
didn't approach it from the viewpoint of providing VM disk images as it
seemed to be more "general" provide the ability to pick-n-choose which
mount option to use. Side bar, I also have this weird recollection about
usage of rsize=n and/or wsize=n in some previous NFS issue I've chased
(I think it was at my previous employer).

There's also mention of noexec in one of the private comments, so while
nosuid and nodev would be easier, it still doesn't cover everything
noted in the bz.

> 
> We could none the less also still have a private XML namespace for
> doing arbitrary mount argument passthrough, but that shouldn't be
> the solution for this particular BZ. It is just a way to get a quick
> workaround for an option while we decide how to model the desired
> new mount option explicitly in the XML, as we do for QEMU options.
> 

Does it become more work than it's worth though ;-)  How large of a wall
needs to be built to stop the flow of code ;-)

John

>>
>>>
>>> Anything that is implemented via a separate XML namespace is
>>> generally considered
>>>
>>>    "unsupported, you're on your own when it breaks"
>>>
>>> so if this feature is neeeded for any management application like
>>> oVirt, OpenStack, etc, then using a custom namespace is not going
>>> to be a suitable approach.
>>
>> And I don't think it would be palatable to add N 'text' options to the
>> XML that map to the same N 'text' options in the mount command. So this
>> was the next best option as far as I saw it. That'd be one of those
>> never ending tasks to add the favorite option.
> 
> Regards,
> Daniel
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by Daniel P. Berrangé 5 years, 3 months ago
On Wed, Jan 09, 2019 at 01:26:19PM -0500, John Ferlan wrote:
> 
> 
> On 1/9/19 12:40 PM, Daniel P. Berrangé wrote:
> > On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote:
> >>
> >>
> >> On 1/9/19 12:09 PM, Daniel P. Berrangé wrote:
> >>> On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote:
> >>>> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html
> >>>>
> >>>> Kept the subject the same, but the concept has been adjusted to follow
> >>>> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML.
> >>>> This series adds both the NFS and the RBD adjustments that were essentially
> >>>> in the referenced series from review.
> >>>
> >>> Can you give any info about what motivated this addition.
> >>
> >> There's a bz:
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1584663
> >>
> >> which along the way has had private comments - I probably should add the
> >> bz to at least one of the patches, but I think because of the (probably
> >> unnecessary) private comments in the bz, I was hesitant to do so.
> >>
> >> I tried a different mechanism and Jano pointed out:
> >>
> >> https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html
> >>
> >> that previous attempts were rejected due to the arbitrary text.
> > 
> > Ok, from the rather limited information in the BZ above, it is clear
> > that this feature is required to be production supported, so that
> > rules out use of private XML namespaces as a viable solution.
> > 
> > I agree with Jano's rejected of arbitrary mount option passthrough in
> > v1 too though.
> 
> I understand the rejection and I also found namespaces to not be very
> user or developer friendly. Even less easy to understand were those
> "other' namespaces using "ns0" and/or "ns1". Proverbial rock and hard
> place scenario.
> 
> It's no small wonder why the patches that were rejected in June 2014 had
> no attempted followup that I found - it just didn't seem worth the
> effort and using their own private code to suit their own needs was far
> more simple!
> 
> > 
> > I think the right answer involves multiple approaches in the XML.
> > 
> > For the NFS pool, we should have an explicit way to request the NFS
> > protocol version in the XML.
> 
> You mean mount option "nfsvers=n"?

Yes,  which would be set using something like  

   <source>
     ....
     <protocol ver="4"/>
   </source>

> > For the general nosuid, nodev flags, I think we can do something like we
> > have for the <features/> element in the domain XML.
> > 
> 
> So that means to me that every option currently possible in mount would
> need to be listed.  Doesn't that feel excessive?  Not that mount changes
> that often, but I would think it's awfully painful to take that route.
> Especially for some of the "more complicated" mount options.

The mount command options are a tiny, trivial set compared to QEMU
command line options. The goal here is not to have simple libvirt
code, rather is to have portable & simple application code. Libvirt
exists to provide an abstraction layer over OS specific commands
line "mount" which can have different syntax on each OS. eg FreeBSD
mount command options may differ from Linux mount command options.

I'm not suggesting implementing support for every single mount
option though. Only those that we actually have a compelling
reason to support, just as we've not implemented every single
QEMU arg.

> > Or on second thoughts, the only reason for the storage pools mounts is to
> > provide VM disk images, so we should just unconditionally always set nosuid
> > & nodev when running mount. No storage volume we report should be setuid or
> > be a device node.  If the NFS mount has a directory tree for container
> > filesystems, then the previously discussed  virFileSystem APIs should be
> > actually implemented.
> 
> That'd be the really simple simple fix at least for nosuid and nodev. I
> didn't approach it from the viewpoint of providing VM disk images as it
> seemed to be more "general" provide the ability to pick-n-choose which
> mount option to use. Side bar, I also have this weird recollection about
> usage of rsize=n and/or wsize=n in some previous NFS issue I've chased
> (I think it was at my previous employer).

That was needed in NFSv2, but in modern NFSv3/4 IIUC the server decides
those values.

> There's also mention of noexec in one of the private comments, so while
> nosuid and nodev would be easier, it still doesn't cover everything
> noted in the bz.

noexec would be reasonable to turn on by default too IMHO.

> > We could none the less also still have a private XML namespace for
> > doing arbitrary mount argument passthrough, but that shouldn't be
> > the solution for this particular BZ. It is just a way to get a quick
> > workaround for an option while we decide how to model the desired
> > new mount option explicitly in the XML, as we do for QEMU options.
> > 
> 
> Does it become more work than it's worth though ;-)  How large of a wall
> needs to be built to stop the flow of code ;-)


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
Re: [libvirt] [PATCH v2 0/6] Allow adding mountOpts to the storage pool mount command
Posted by John Ferlan 5 years, 3 months ago
[...]

>>> For the general nosuid, nodev flags, I think we can do something like we
>>> have for the <features/> element in the domain XML.
>>>
>>
>> So that means to me that every option currently possible in mount would
>> need to be listed.  Doesn't that feel excessive?  Not that mount changes
>> that often, but I would think it's awfully painful to take that route.
>> Especially for some of the "more complicated" mount options.
> 
> The mount command options are a tiny, trivial set compared to QEMU
> command line options. The goal here is not to have simple libvirt
> code, rather is to have portable & simple application code. Libvirt
> exists to provide an abstraction layer over OS specific commands
> line "mount" which can have different syntax on each OS. eg FreeBSD
> mount command options may differ from Linux mount command options.

And this last line is the biggest concern over providing or "by default"
adding "-o nosuid,nodev,noexec" for NFS mount command line.

Secondary to that is the "what if" by doing so we break some existing
configuration for some reason that was making use of a pool volume in
some really strange manner that we didn't expect just because one of
those mount options wasn't set by default. Not that it seems the 3
mentioned ones would cause such pain, but the precedence of adding
options by default in general.

> 
> I'm not suggesting implementing support for every single mount
> option though. Only those that we actually have a compelling
> reason to support, just as we've not implemented every single
> QEMU arg.
> 

Understood. Of course as soon a couple are implemented, someone else's
favorite is desired, etc. It's the picking and choosing which are
compelling enough to support vs. just allowing or even providing a
supportable mechanism to allow arbitrary command options vs.
implementing "some" and leaving the rest to be added via namespace.

Right now the "some" are:

    nfsvers='n'  [add only if new XML exists]
    nosuid
    nodev
    noexec

and maybe additionally 'ro' if [new] <readonly/> was set.

Just trying to get all the "parameters" set before starting a v3...

John

[...]

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