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@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);
}
diff --git a/tests/storagepoolxml2argvdata/pool-netfs-auto-freebsd.argv b/tests/storagepoolxml2argvdata/pool-netfs-auto-freebsd.argv
new file mode 100644
index 0000000000..39e5c97aed
--- /dev/null
+++ b/tests/storagepoolxml2argvdata/pool-netfs-auto-freebsd.argv
@@ -0,0 +1 @@
+mount localhost:/var/lib/libvirt/images /mnt -o nosuid,noexec
diff --git a/tests/storagepoolxml2argvdata/pool-netfs-auto-linux.argv b/tests/storagepoolxml2argvdata/pool-netfs-auto-linux.argv
new file mode 100644
index 0000000000..1f82d3d29c
--- /dev/null
+++ b/tests/storagepoolxml2argvdata/pool-netfs-auto-linux.argv
@@ -0,0 +1 @@
+mount localhost:/var/lib/libvirt/images /mnt -o nodev,nosuid,noexec
diff --git a/tests/storagepoolxml2argvdata/pool-netfs-freebsd.argv b/tests/storagepoolxml2argvdata/pool-netfs-freebsd.argv
new file mode 100644
index 0000000000..05c1951f32
--- /dev/null
+++ b/tests/storagepoolxml2argvdata/pool-netfs-freebsd.argv
@@ -0,0 +1 @@
+mount -t nfs localhost:/var/lib/libvirt/images /mnt -o nosuid,noexec
diff --git a/tests/storagepoolxml2argvdata/pool-netfs-linux.argv b/tests/storagepoolxml2argvdata/pool-netfs-linux.argv
new file mode 100644
index 0000000000..22fafd7b32
--- /dev/null
+++ b/tests/storagepoolxml2argvdata/pool-netfs-linux.argv
@@ -0,0 +1 @@
+mount -t nfs localhost:/var/lib/libvirt/images /mnt -o nodev,nosuid,noexec
diff --git a/tests/storagepoolxml2argvtest.c b/tests/storagepoolxml2argvtest.c
index 2f2d40e027..c114fecbcf 100644
--- a/tests/storagepoolxml2argvtest.c
+++ b/tests/storagepoolxml2argvtest.c
@@ -96,6 +96,8 @@ testCompareXMLToArgvFiles(bool shouldFail,
struct testInfo {
bool shouldFail;
const char *pool;
+ bool linuxOut;
+ bool freebsdOut;
};
static int
@@ -110,9 +112,19 @@ testCompareXMLToArgvHelper(const void *data)
abs_srcdir, info->pool) < 0)
goto cleanup;
- if (virAsprintf(&cmdline, "%s/storagepoolxml2argvdata/%s.argv",
- abs_srcdir, info->pool) < 0 && !info->shouldFail)
- goto cleanup;
+ if (info->linuxOut) {
+ if (virAsprintf(&cmdline, "%s/storagepoolxml2argvdata/%s-linux.argv",
+ abs_srcdir, info->pool) < 0 && !info->shouldFail)
+ goto cleanup;
+ } else if (info->freebsdOut) {
+ if (virAsprintf(&cmdline, "%s/storagepoolxml2argvdata/%s-freebsd.argv",
+ abs_srcdir, info->pool) < 0 && !info->shouldFail)
+ goto cleanup;
+ } else {
+ if (virAsprintf(&cmdline, "%s/storagepoolxml2argvdata/%s.argv",
+ abs_srcdir, info->pool) < 0 && !info->shouldFail)
+ goto cleanup;
+ }
result = testCompareXMLToArgvFiles(info->shouldFail, poolxml, cmdline);
@@ -129,9 +141,9 @@ mymain(void)
{
int ret = 0;
-#define DO_TEST_FULL(shouldFail, pool) \
+#define DO_TEST_FULL(shouldFail, pool, linuxOut, freebsdOut) \
do { \
- struct testInfo info = { shouldFail, pool }; \
+ struct testInfo info = { shouldFail, pool, linuxOut, freebsdOut }; \
if (virTestRun("Storage Pool XML-2-argv " pool, \
testCompareXMLToArgvHelper, &info) < 0) \
ret = -1; \
@@ -139,10 +151,16 @@ mymain(void)
while (0);
#define DO_TEST(pool, ...) \
- DO_TEST_FULL(false, pool)
+ DO_TEST_FULL(false, pool, false, false)
#define DO_TEST_FAIL(pool, ...) \
- DO_TEST_FULL(true, pool)
+ DO_TEST_FULL(true, pool, false, false)
+
+#define DO_TEST_LINUX(pool, ...) \
+ DO_TEST_FULL(false, pool, true, false)
+
+#define DO_TEST_FREEBSD(pool, ...) \
+ DO_TEST_FULL(false, pool, false, true)
DO_TEST_FAIL("pool-dir");
DO_TEST_FAIL("pool-dir-naming");
@@ -155,8 +173,16 @@ mymain(void)
DO_TEST_FAIL("pool-disk-device-nopartsep");
DO_TEST_FAIL("pool-iscsi");
DO_TEST_FAIL("pool-iscsi-auth");
+#ifdef __linux__
+ DO_TEST_LINUX("pool-netfs");
+ DO_TEST_LINUX("pool-netfs-auto");
+#elif defined(__FreeBSD__)
+ DO_TEST_FREEBSD("pool-netfs");
+ DO_TEST_FREEBSD("pool-netfs-auto");
+#else
DO_TEST("pool-netfs");
DO_TEST("pool-netfs-auto");
+#endif
DO_TEST("pool-netfs-gluster");
DO_TEST("pool-netfs-cifs");
DO_TEST_FAIL("pool-scsi");
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
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
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@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);
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 29, 2019 at 08:43:31AM -0500, John Ferlan wrote:
>
>
> 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@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.
I mentioned later for patch 9 that the namespace options should apply for
any filesystem mounts we do IOW, both pool type=fs and type=netfs. In
fact even for type=netfs, we can't assume it is NFS - it could be a CIFS
or GlusterFS pool, rather than NFS so the name is a bit misleading in
that respect anyway.
For patch 4, we just need to push the check for whether it is NFS
down into the virStorageBackendFileSystemMountAddOptions method,
so it only formats the nfsvers for that specific pool config.
Presumably the XML parser stops us even parsing the protocol out
of the XML unless type == netfs + format==nfs, so we'd be ok even
without the check, but its less suprising to have the explicit
check anyway.
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
© 2016 - 2026 Red Hat, Inc.