Re: [libvirt] [PATCH v4 01/10] storage: Add default mount options for NFS Storage Pools

On 1/29/19 6:44 AM, Daniel P. Berrangé wrote: > On Thu, Jan 17, 2019 at 04:22:07PM -0500, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1584663 >> >> Modify the command generation to add some default options to >> an NFS Storage Pool based on the OS type. For Linux, it'll be >> the "nodev, nosuid, noexec". For FreeBSD, it'll be "nosuid, noexec". >> For others, just leave the options alone. >> >> Modify the storagepoolxml2argvtest to handle the fact that the >> same input XML could generate different output XML based on whether >> Linux, FreeBSD, or other was being built. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/storage/storage_util.c | 16 ++++++++ >> .../pool-netfs-auto-freebsd.argv | 1 + >> .../pool-netfs-auto-linux.argv | 1 + >> .../pool-netfs-freebsd.argv | 1 + >> .../pool-netfs-linux.argv | 1 + >> tests/storagepoolxml2argvtest.c | 40 +++++++++++++++---- >> 6 files changed, 53 insertions(+), 7 deletions(-) >> create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-auto-freebsd.argv >> create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-auto-linux.argv >> create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-freebsd.argv >> create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-linux.argv >> >> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c >> index a84ee5b600..44a95d9fab 100644 >> --- a/src/storage/storage_util.c >> +++ b/src/storage/storage_util.c >> @@ -34,6 +34,11 @@ >> # ifndef FS_NOCOW_FL >> # define FS_NOCOW_FL 0x00800000 /* Do not cow file */ >> # endif >> +# define default_nfs_mount_opts "nodev,nosuid,noexec" >> +#elif defined(__FreeBSD__) >> +# define default_nfs_mount_opts "nosuid,noexec" >> +#else >> +# define default_nfs_mount_opts "" >> #endif >> >> #if WITH_BLKID >> @@ -4261,12 +4266,21 @@ virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool) >> } >> >> >> +static void >> +virStorageBackendFileSystemMountNFSAddOptions(virCommandPtr cmd) >> +{ >> + if (*default_nfs_mount_opts != '\0') >> + virCommandAddArgList(cmd, "-o", default_nfs_mount_opts, NULL); >> +} >> + >> + >> static void >> virStorageBackendFileSystemMountNFSArgs(virCommandPtr cmd, >> const char *src, >> virStoragePoolDefPtr def) >> { >> virCommandAddArgList(cmd, src, def->target.path, NULL); >> + virStorageBackendFileSystemMountNFSAddOptions(cmd); >> } >> >> >> @@ -4308,6 +4322,8 @@ virStorageBackendFileSystemMountDefaultArgs(virCommandPtr cmd, >> else >> fmt = virStoragePoolFormatFileSystemNetTypeToString(def->source.format); >> virCommandAddArgList(cmd, "-t", fmt, src, def->target.path, NULL); >> + if (def->type == VIR_STORAGE_POOL_NETFS) >> + virStorageBackendFileSystemMountNFSAddOptions(cmd); > > This doesn't need to be restricted to just NFS. A kind of filesystem we mount > as a storage pool should use these extra options, as they are general purpose > mount options, not FS specific. > > Just s/nfs//i in all the method names / constants. > I can modify default_nfs_mount_opts to be just default_mount_opts; however, as you see going forward the new method ends up being used by patch4 for "nfsvers=%u," and patch9 for Namespace options, so I thinking keeping the name virStorageBackendFileSystemMountNFSAddOptions is better.