[libvirt] [PATCH] Make ZFS storage pool XML tests optional
From: Gary R Hook <grhookatwork@gmail.com> Do not run ZFS tests when ZFS is unsupported in the environment. The recent patch b4af40226d09adeaf9e33a1d6594c4e8ce7f771d adds tests to storagepoolxml2xmltest for the optional ZFS feature. When ZFS is not included in the configuration these tests should not / cannot be run. Modify Makefile.am and the test source file to check for use of the feature and compile in the tests accordingly. Signed-off-by: Gary R Hook <gary.hook@nimboxx.com> --- tests/Makefile.am | 4 ++++ tests/storagepoolxml2xmltest.c | 2 ++ 2 files changed, 6 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index e9418ea..8f8df27 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -780,6 +780,10 @@ storagepoolxml2xmltest_SOURCES = \ storagepoolxml2xmltest.c \ testutils.c testutils.h storagepoolxml2xmltest_LDADD = $(LDADDS) +storagepoolxml2xmltest_CFLAGS = $(AM_CFLAGS) +if WITH_STORAGE_ZFS +storagepoolxml2xmltest_CFLAGS += -DWITH_STORAGE_ZFS +endif nodedevxml2xmltest_SOURCES = \ nodedevxml2xmltest.c \ diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 52e2193..270f75d 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -106,8 +106,10 @@ mymain(void) DO_TEST("pool-gluster"); DO_TEST("pool-gluster-sub"); DO_TEST("pool-scsi-type-scsi-host-stable"); +#ifdef WITH_STORAGE_ZFS DO_TEST("pool-zfs"); DO_TEST("pool-zfs-sourcedev"); +#endif return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.9.1
On 01/15/2015 03:32 PM, Gary R Hook wrote:
From: Gary R Hook <grhookatwork@gmail.com>
Do not run ZFS tests when ZFS is unsupported in the environment.
The recent patch b4af40226d09adeaf9e33a1d6594c4e8ce7f771d adds tests to storagepoolxml2xmltest for the optional ZFS feature. When ZFS is not included in the configuration these tests should not / cannot be run. Modify Makefile.am and the test source file to check for use of the feature and compile in the tests accordingly.
Signed-off-by: Gary R Hook <gary.hook@nimboxx.com>
--- tests/Makefile.am | 4 ++++ tests/storagepoolxml2xmltest.c | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/tests/Makefile.am b/tests/Makefile.am index e9418ea..8f8df27 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -780,6 +780,10 @@ storagepoolxml2xmltest_SOURCES = \ storagepoolxml2xmltest.c \ testutils.c testutils.h storagepoolxml2xmltest_LDADD = $(LDADDS) +storagepoolxml2xmltest_CFLAGS = $(AM_CFLAGS) +if WITH_STORAGE_ZFS +storagepoolxml2xmltest_CFLAGS += -DWITH_STORAGE_ZFS +endif
This shouldn't be needed. Our config.h should already be defining all the appropriate WITH_* feature macros. If it is not, that's a bug in configure.ac.
nodedevxml2xmltest_SOURCES = \ nodedevxml2xmltest.c \ diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 52e2193..270f75d 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -106,8 +106,10 @@ mymain(void) DO_TEST("pool-gluster"); DO_TEST("pool-gluster-sub"); DO_TEST("pool-scsi-type-scsi-host-stable"); +#ifdef WITH_STORAGE_ZFS DO_TEST("pool-zfs"); DO_TEST("pool-zfs-sourcedev"); +#endif
Why is only this test conditional? Shouldn't other things like gluster also be conditional, based on whether those features were built into libvirt? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 1/15/15 4:37 PM, Eric Blake wrote:
On 01/15/2015 03:32 PM, Gary R Hook wrote:
From: Gary R Hook <grhookatwork@gmail.com>
Do not run ZFS tests when ZFS is unsupported in the environment.
The recent patch b4af40226d09adeaf9e33a1d6594c4e8ce7f771d adds tests to storagepoolxml2xmltest for the optional ZFS feature. When ZFS is not included in the configuration these tests should not / cannot be run. Modify Makefile.am and the test source file to check for use of the feature and compile in the tests accordingly.
Signed-off-by: Gary R Hook <gary.hook@nimboxx.com>
--- tests/Makefile.am | 4 ++++ tests/storagepoolxml2xmltest.c | 2 ++ 2 files changed, 6 insertions(+)
diff --git a/tests/Makefile.am b/tests/Makefile.am index e9418ea..8f8df27 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -780,6 +780,10 @@ storagepoolxml2xmltest_SOURCES = \ storagepoolxml2xmltest.c \ testutils.c testutils.h storagepoolxml2xmltest_LDADD = $(LDADDS) +storagepoolxml2xmltest_CFLAGS = $(AM_CFLAGS) +if WITH_STORAGE_ZFS +storagepoolxml2xmltest_CFLAGS += -DWITH_STORAGE_ZFS +endif
This shouldn't be needed. Our config.h should already be defining all the appropriate WITH_* feature macros. If it is not, that's a bug in configure.ac.
I'm not adding a #define. I'm just passing one along to the compiler. Turning a configure parameter in to a compiler parameter. If you have a suggestion as to how to #ifdef the source code (below) in a way that keeps it from being compiled when ZFS isn't in use, I'm all ears. I thought this was pretty clean.
nodedevxml2xmltest_SOURCES = \ nodedevxml2xmltest.c \ diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 52e2193..270f75d 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -106,8 +106,10 @@ mymain(void) DO_TEST("pool-gluster"); DO_TEST("pool-gluster-sub"); DO_TEST("pool-scsi-type-scsi-host-stable"); +#ifdef WITH_STORAGE_ZFS DO_TEST("pool-zfs"); DO_TEST("pool-zfs-sourcedev"); +#endif
Why is only this test conditional? Shouldn't other things like gluster also be conditional, based on whether those features were built into libvirt?
You have a very good point. I wasn't trying to fix any/all problems, just the one that bit me, and that is the most recent of this type of mistake. I might conclude that any older conditional tests haven't been exposed as problematic, although clearly every test should be properly vetted against the actual environment. Really appreciate the quick notice and response, however. Thank you. -- Gary R Hook Senior Kernel Engineer NIMBOXX, Inc
On 01/15/2015 03:53 PM, Gary R Hook wrote:
+storagepoolxml2xmltest_CFLAGS = $(AM_CFLAGS) +if WITH_STORAGE_ZFS +storagepoolxml2xmltest_CFLAGS += -DWITH_STORAGE_ZFS +endif
This shouldn't be needed. Our config.h should already be defining all the appropriate WITH_* feature macros. If it is not, that's a bug in configure.ac.
I'm not adding a #define. I'm just passing one along to the compiler. Turning a configure parameter in to a compiler parameter.
Adding a -D to Makefile.am is the same as adding a #define to the source; my point was that the source should already have the #define without the need for a -D in CFLAGS.
If you have a suggestion as to how to #ifdef the source code (below) in a way that keeps it from being compiled when ZFS isn't in use, I'm all ears. I thought this was pretty clean.
After a quick search, I note that src/storage/storage_backend.c already uses WITH_STORAGE_ZFS without having to add a -D to the CFLAGS, because <config.h> already defines it. So your testcase should do the same. You shouldn't have to modify Makefile.am; just blindly use WITH_STORAGE_ZFS in the test file, which already includes <config.h>.
nodedevxml2xmltest_SOURCES = \ nodedevxml2xmltest.c \ diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index 52e2193..270f75d 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -106,8 +106,10 @@ mymain(void) DO_TEST("pool-gluster"); DO_TEST("pool-gluster-sub"); DO_TEST("pool-scsi-type-scsi-host-stable"); +#ifdef WITH_STORAGE_ZFS DO_TEST("pool-zfs"); DO_TEST("pool-zfs-sourcedev"); +#endif
Why is only this test conditional? Shouldn't other things like gluster also be conditional, based on whether those features were built into libvirt?
You have a very good point. I wasn't trying to fix any/all problems, just the one that bit me, and that is the most recent of this type of mistake. I might conclude that any older conditional tests haven't been exposed as problematic, although clearly every test should be properly vetted against the actual environment.
Your solution seems okay to me, except that we probably ought to make all of the tests honor the proper conditionals, and not just the new zfs test.
Really appreciate the quick notice and response, however. Thank you.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 1/15/15 5:17 PM, Eric Blake wrote:
Your solution seems okay to me, except that we probably ought to make all of the tests honor the proper conditionals, and not just the new zfs test.
I agree. But that can be said for all of tests, not just this one. Perhaps future patch acceptance can keep this issue in mind; clean-up of the existing tests is not really my circus. -- Gary R Hook Senior Kernel Engineer NIMBOXX, Inc
participants (2)
-
Eric Blake -
Gary R Hook