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

[John Ferlan]

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 at redhat.com>
>> ---
>>  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.

Fair enough?

John

With diffs squashed in:

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 44a95d9fab..8d78162a5a 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -34,11 +34,11 @@
 # ifndef FS_NOCOW_FL
 #  define FS_NOCOW_FL                     0x00800000 /* Do not cow file */
 # endif
-# define default_nfs_mount_opts "nodev,nosuid,noexec"
+# define default_mount_opts "nodev,nosuid,noexec"
 #elif defined(__FreeBSD__)
-# define default_nfs_mount_opts "nosuid,noexec"
+# define default_mount_opts "nosuid,noexec"
 #else
-# define default_nfs_mount_opts ""
+# define default_mount_opts ""
 #endif

 #if WITH_BLKID
@@ -4269,8 +4269,8 @@
virStorageBackendFileSystemGetPoolSource(virStoragePoolObjPtr pool)
 static void
 virStorageBackendFileSystemMountNFSAddOptions(virCommandPtr cmd)
 {
-    if (*default_nfs_mount_opts != '\0')
-        virCommandAddArgList(cmd, "-o", default_nfs_mount_opts, NULL);
+    if (*default_mount_opts != '\0')
+        virCommandAddArgList(cmd, "-o", default_mount_opts, NULL);
 }