| Message ID | 1410882204-9836-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 16, 2014 at 05:43:24PM +0200, Paolo Bonzini wrote: > Glib recently introduced a robust way to run tests in a subprocess, > which is used in test-qdev-global-props. However, we would like > to have the same tests run with older versions of glib, and the > older fork-based mechanisms works well enough. > > This still requires: > > - bumping the minimum required version from 2.12 to 2.16. I suggest > bumping to the currently required version for Windows, which is 2.20 > (released March 2009). > > - disabling the test on Windows, since it does not have fork Right but it could work on windows if glib is new enough? Why disable there? > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tests/Makefile | 2 +- > tests/test-qdev-global-props.c | 63 +++++++++++++++++++++++++++--------------- > 2 files changed, 41 insertions(+), 24 deletions(-) > > diff --git a/tests/Makefile b/tests/Makefile > index d5db97b..7f125e8 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -58,7 +58,7 @@ check-unit-y += tests/test-int128$(EXESUF) > # all code tested by test-int128 is inside int128.h > gcov-files-test-int128-y = > check-unit-y += tests/test-bitops$(EXESUF) > -check-unit-y += tests/test-qdev-global-props$(EXESUF) > +check-unit-$(CONFIG_POSIX) += tests/test-qdev-global-props$(EXESUF) > check-unit-y += tests/check-qom-interface$(EXESUF) > gcov-files-check-qom-interface-y = qom/object.c > check-unit-$(CONFIG_POSIX) += tests/test-vmstate$(EXESUF) > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c > index 0be9835..8e4b270 100644 > --- a/tests/test-qdev-global-props.c > +++ b/tests/test-qdev-global-props.c > @@ -29,6 +29,27 @@ > #include "qom/object.h" > #include "qapi/visitor.h" > > +/* Use the older fork() mechanism if the newer, more robust subprocess tests > + * are not available. > + */ > +#if GLIB_CHECK_VERSION(2, 38, 0) > +# define TEST_ADD_FUNC_SUBPROCESS(name, func) do { \ > + g_test_add_func(name "/subprocess", func##_subprocess); \ > + g_test_add_func(name, func); \ > + } while (0) > +# define TEST_TRAP_SUBPROCESS(name, func) \ > + g_test_trap_subprocess(name, 0, 0) > +#else > +# define TEST_ADD_FUNC_SUBPROCESS(name, func) \ > + g_test_add_func(name, func) > +# define TEST_TRAP_SUBPROCESS(name, func) do { \ > + if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDOUT | G_TEST_TRAP_SILENCE_STDERR)) { \ > + func(); \ > + exit(0); \ > + } \ > + } while(0) > +#endif > + > two empty lines. > #define TYPE_STATIC_PROPS "static_prop_type" > #define STATIC_TYPE(obj) \ > @@ -77,7 +98,8 @@ static void test_static_prop_subprocess(void) > > static void test_static_prop(void) > { > - g_test_trap_subprocess("/qdev/properties/static/default/subprocess", 0, 0); > + TEST_TRAP_SUBPROCESS("/qdev/properties/static/default/subprocess", > + test_static_prop_subprocess); > g_test_trap_assert_passed(); > g_test_trap_assert_stderr(""); > g_test_trap_assert_stdout(""); > @@ -103,7 +125,8 @@ static void test_static_globalprop_subprocess(void) > > static void test_static_globalprop(void) > { > - g_test_trap_subprocess("/qdev/properties/static/global/subprocess", 0, 0); > + TEST_TRAP_SUBPROCESS("/qdev/properties/static/global/subprocess", > + test_static_globalprop_subprocess); > g_test_trap_assert_passed(); > g_test_trap_assert_stderr(""); > g_test_trap_assert_stdout(""); > @@ -235,7 +258,8 @@ static void test_dynamic_globalprop_subprocess(void) > > static void test_dynamic_globalprop(void) > { > - g_test_trap_subprocess("/qdev/properties/dynamic/global/subprocess", 0, 0); > + TEST_TRAP_SUBPROCESS("/qdev/properties/dynamic/global/subprocess", > + test_dynamic_globalprop_subprocess); > g_test_trap_assert_passed(); > g_test_trap_assert_stderr_unmatched("*prop1*"); > g_test_trap_assert_stderr_unmatched("*prop2*"); > @@ -280,7 +304,8 @@ static void test_dynamic_globalprop_nouser_subprocess(void) > > static void test_dynamic_globalprop_nouser(void) > { > - g_test_trap_subprocess("/qdev/properties/dynamic/global/nouser/subprocess", 0, 0); > + TEST_TRAP_SUBPROCESS("/qdev/properties/dynamic/global/nouser/subprocess", > + test_dynamic_globalprop_nouser_subprocess); > g_test_trap_assert_passed(); > g_test_trap_assert_stderr(""); > g_test_trap_assert_stdout(""); > @@ -297,25 +322,17 @@ int main(int argc, char **argv) > type_register_static(&nohotplug_type); > type_register_static(&nondevice_type); > > - g_test_add_func("/qdev/properties/static/default/subprocess", > - test_static_prop_subprocess); > - g_test_add_func("/qdev/properties/static/default", > - test_static_prop); > - > - g_test_add_func("/qdev/properties/static/global/subprocess", > - test_static_globalprop_subprocess); > - g_test_add_func("/qdev/properties/static/global", > - test_static_globalprop); > - > - g_test_add_func("/qdev/properties/dynamic/global/subprocess", > - test_dynamic_globalprop_subprocess); > - g_test_add_func("/qdev/properties/dynamic/global", > - test_dynamic_globalprop); > - > - g_test_add_func("/qdev/properties/dynamic/global/nouser/subprocess", > - test_dynamic_globalprop_nouser_subprocess); > - g_test_add_func("/qdev/properties/dynamic/global/nouser", > - test_dynamic_globalprop_nouser); > + TEST_ADD_FUNC_SUBPROCESS("/qdev/properties/static/default", > + test_static_prop); > + > + TEST_ADD_FUNC_SUBPROCESS("/qdev/properties/static/global", > + test_static_globalprop); > + > + TEST_ADD_FUNC_SUBPROCESS("/qdev/properties/dynamic/global", > + test_dynamic_globalprop); > + > + TEST_ADD_FUNC_SUBPROCESS("/qdev/properties/dynamic/global/nouser", > + test_dynamic_globalprop_nouser); > > g_test_run(); > > -- > 2.1.0
On 16 September 2014 08:43, Paolo Bonzini <pbonzini@redhat.com> wrote: > Glib recently introduced a robust way to run tests in a subprocess, > which is used in test-qdev-global-props. However, we would like > to have the same tests run with older versions of glib, and the > older fork-based mechanisms works well enough. > > This still requires: > > - bumping the minimum required version from 2.12 to 2.16. I suggest > bumping to the currently required version for Windows, which is 2.20 > (released March 2009). The commit message for a52d28afb suggests that this bump would be dropping support for RHEL5. That doesn't seem like a great idea to me, especially if it's only for the benefit of a test program. On the other hand your patch doesn't actually bump the required version, so I'm a bit confused. -- PMM
Il 16/09/2014 18:20, Peter Maydell ha scritto: >> > - bumping the minimum required version from 2.12 to 2.16. I suggest >> > bumping to the currently required version for Windows, which is 2.20 >> > (released March 2009). > The commit message for a52d28afb suggests that this bump would be > dropping support for RHEL5. That doesn't seem like a great idea > to me, especially if it's only for the benefit of a test program. Though you would just drop support for "make check" on RHEL5. Even "make -k check" would roughly work. > On the other hand your patch doesn't actually bump the required > version, so I'm a bit confused. Yeah, this is just an RFC. Paolo
On 16 September 2014 09:23, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 16/09/2014 18:20, Peter Maydell ha scritto: >>> > - bumping the minimum required version from 2.12 to 2.16. I suggest >>> > bumping to the currently required version for Windows, which is 2.20 >>> > (released March 2009). >> The commit message for a52d28afb suggests that this bump would be >> dropping support for RHEL5. That doesn't seem like a great idea >> to me, especially if it's only for the benefit of a test program. > > Though you would just drop support for "make check" on RHEL5. Even > "make -k check" would roughly work. Can't we just put in the makefile and configure magic to skip the test if the glib version is too old for it? -- PMM
Il 16/09/2014 18:28, Peter Maydell ha scritto: >> > Though you would just drop support for "make check" on RHEL5. Even >> > "make -k check" would roughly work. > Can't we just put in the makefile and configure magic to skip > the test if the glib version is too old for it? Yes, that's what Michael did (minus the magic to use the older fork-based code). But is it really worthwhile to support RHEL5? We made glib mandatory in 2011 (around RHEL6.2), and that broke RHEL4. By the time the next QEMU release is ready RHEL7.1 should be out, give or take a month or so. Paolo
On Tue, Sep 16, 2014 at 06:23:29PM +0200, Paolo Bonzini wrote: > Il 16/09/2014 18:20, Peter Maydell ha scritto: > >> > - bumping the minimum required version from 2.12 to 2.16. I suggest > >> > bumping to the currently required version for Windows, which is 2.20 > >> > (released March 2009). > > The commit message for a52d28afb suggests that this bump would be > > dropping support for RHEL5. That doesn't seem like a great idea > > to me, especially if it's only for the benefit of a test program. > > Though you would just drop support for "make check" on RHEL5. Even > "make -k check" would roughly work. Probably not too hard to drop the specific test, right? > > On the other hand your patch doesn't actually bump the required > > version, so I'm a bit confused. > > Yeah, this is just an RFC. > > Paolo Message in the glib header hints at some reliability problems with g_test_fork. Any idea what these might be?
On Tue, Sep 16, 2014 at 09:28:03AM -0700, Peter Maydell wrote: > On 16 September 2014 09:23, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 16/09/2014 18:20, Peter Maydell ha scritto: > >>> > - bumping the minimum required version from 2.12 to 2.16. I suggest > >>> > bumping to the currently required version for Windows, which is 2.20 > >>> > (released March 2009). > >> The commit message for a52d28afb suggests that this bump would be > >> dropping support for RHEL5. That doesn't seem like a great idea > >> to me, especially if it's only for the benefit of a test program. > > > > Though you would just drop support for "make check" on RHEL5. Even > > "make -k check" would roughly work. > > Can't we just put in the makefile and configure magic to skip > the test if the glib version is too old for it? > > -- PMM That's exactly what my patch did. See 20140916144324.GA7804@redhat.com You prefer that then?
On 16 September 2014 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 16/09/2014 18:28, Peter Maydell ha scritto: >>> > Though you would just drop support for "make check" on RHEL5. Even >>> > "make -k check" would roughly work. >> Can't we just put in the makefile and configure magic to skip >> the test if the glib version is too old for it? > > Yes, that's what Michael did (minus the magic to use the older > fork-based code). But is it really worthwhile to support RHEL5? We > made glib mandatory in 2011 (around RHEL6.2), and that broke RHEL4. By > the time the next QEMU release is ready RHEL7.1 should be out, give or > take a month or so. I care because in the EDA tools space things move very slowly and so RHEL5 is still fairly commonplace. Obviously at some point we're going to end up dropping support, but "one test case won't build" seems like a pretty trivial reason to drop it to me. -- PMM
On Tue, Sep 16, 2014 at 09:45:03AM -0700, Peter Maydell wrote: > On 16 September 2014 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 16/09/2014 18:28, Peter Maydell ha scritto: > >>> > Though you would just drop support for "make check" on RHEL5. Even > >>> > "make -k check" would roughly work. > >> Can't we just put in the makefile and configure magic to skip > >> the test if the glib version is too old for it? > > > > Yes, that's what Michael did (minus the magic to use the older > > fork-based code). But is it really worthwhile to support RHEL5? We > > made glib mandatory in 2011 (around RHEL6.2), and that broke RHEL4. By > > the time the next QEMU release is ready RHEL7.1 should be out, give or > > take a month or so. > > I care because in the EDA tools space things move very slowly > and so RHEL5 is still fairly commonplace. Obviously at some > point we're going to end up dropping support, but "one test > case won't build" seems like a pretty trivial reason to drop > it to me. > > -- PMM Could you pls tell me whether my patch works, in your testing? My box which had such an old glib seems to be dead.
On 16 September 2014 09:57, Michael S. Tsirkin <mst@redhat.com> wrote: > Could you pls tell me whether my patch works, in your testing? > My box which had such an old glib seems to be dead. Can you give me a Subject: line or a URL in patchwork, please? (I can't conveniently search by message-id.) thanks -- PMM
On Tue, Sep 16, 2014 at 10:23:03AM -0700, Peter Maydell wrote: > On 16 September 2014 09:57, Michael S. Tsirkin <mst@redhat.com> wrote: > > Could you pls tell me whether my patch works, in your testing? > > My box which had such an old glib seems to be dead. > > Can you give me a Subject: line or a URL in patchwork, > please? (I can't conveniently search by message-id.) > > thanks > -- PMM Gmane supports lookups by ID. This is it: http://mid.gmane.org/20140916144324.GA7804@redhat.com To get raw mailbox, add /raw : http://article.gmane.org/gmane.comp.emulators.qemu/297262/raw OK?
diff --git a/tests/Makefile b/tests/Makefile index d5db97b..7f125e8 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -58,7 +58,7 @@ check-unit-y += tests/test-int128$(EXESUF) # all code tested by test-int128 is inside int128.h gcov-files-test-int128-y = check-unit-y += tests/test-bitops$(EXESUF) -check-unit-y += tests/test-qdev-global-props$(EXESUF) +check-unit-$(CONFIG_POSIX) += tests/test-qdev-global-props$(EXESUF) check-unit-y += tests/check-qom-interface$(EXESUF) gcov-files-check-qom-interface-y = qom/object.c check-unit-$(CONFIG_POSIX) += tests/test-vmstate$(EXESUF) diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c index 0be9835..8e4b270 100644 --- a/tests/test-qdev-global-props.c +++ b/tests/test-qdev-global-props.c @@ -29,6 +29,27 @@ #include "qom/object.h" #include "qapi/visitor.h" +/* Use the older fork() mechanism if the newer, more robust subprocess tests + * are not available. + */ +#if GLIB_CHECK_VERSION(2, 38, 0) +# define TEST_ADD_FUNC_SUBPROCESS(name, func) do { \ + g_test_add_func(name "/subprocess", func##_subprocess); \ + g_test_add_func(name, func); \ + } while (0) +# define TEST_TRAP_SUBPROCESS(name, func) \ + g_test_trap_subprocess(name, 0, 0) +#else +# define TEST_ADD_FUNC_SUBPROCESS(name, func) \ + g_test_add_func(name, func) +# define TEST_TRAP_SUBPROCESS(name, func) do { \ + if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDOUT | G_TEST_TRAP_SILENCE_STDERR)) { \ + func(); \ + exit(0); \ + } \ + } while(0) +#endif + #define TYPE_STATIC_PROPS "static_prop_type" #define STATIC_TYPE(obj) \ @@ -77,7 +98,8 @@ static void test_static_prop_subprocess(void) static void test_static_prop(void) { - g_test_trap_subprocess("/qdev/properties/static/default/subprocess", 0, 0); + TEST_TRAP_SUBPROCESS("/qdev/properties/static/default/subprocess", + test_static_prop_subprocess); g_test_trap_assert_passed(); g_test_trap_assert_stderr(""); g_test_trap_assert_stdout(""); @@ -103,7 +125,8 @@ static void test_static_globalprop_subprocess(void) static void test_static_globalprop(void) { - g_test_trap_subprocess("/qdev/properties/static/global/subprocess", 0, 0); + TEST_TRAP_SUBPROCESS("/qdev/properties/static/global/subprocess", + test_static_globalprop_subprocess); g_test_trap_assert_passed(); g_test_trap_assert_stderr(""); g_test_trap_assert_stdout(""); @@ -235,7 +258,8 @@ static void test_dynamic_globalprop_subprocess(void) static void test_dynamic_globalprop(void) { - g_test_trap_subprocess("/qdev/properties/dynamic/global/subprocess", 0, 0); + TEST_TRAP_SUBPROCESS("/qdev/properties/dynamic/global/subprocess", + test_dynamic_globalprop_subprocess); g_test_trap_assert_passed(); g_test_trap_assert_stderr_unmatched("*prop1*"); g_test_trap_assert_stderr_unmatched("*prop2*"); @@ -280,7 +304,8 @@ static void test_dynamic_globalprop_nouser_subprocess(void) static void test_dynamic_globalprop_nouser(void) { - g_test_trap_subprocess("/qdev/properties/dynamic/global/nouser/subprocess", 0, 0); + TEST_TRAP_SUBPROCESS("/qdev/properties/dynamic/global/nouser/subprocess", + test_dynamic_globalprop_nouser_subprocess); g_test_trap_assert_passed(); g_test_trap_assert_stderr(""); g_test_trap_assert_stdout(""); @@ -297,25 +322,17 @@ int main(int argc, char **argv) type_register_static(&nohotplug_type); type_register_static(&nondevice_type); - g_test_add_func("/qdev/properties/static/default/subprocess", - test_static_prop_subprocess); - g_test_add_func("/qdev/properties/static/default", - test_static_prop); - - g_test_add_func("/qdev/properties/static/global/subprocess", - test_static_globalprop_subprocess); - g_test_add_func("/qdev/properties/static/global", - test_static_globalprop); - - g_test_add_func("/qdev/properties/dynamic/global/subprocess", - test_dynamic_globalprop_subprocess); - g_test_add_func("/qdev/properties/dynamic/global", - test_dynamic_globalprop); - - g_test_add_func("/qdev/properties/dynamic/global/nouser/subprocess", - test_dynamic_globalprop_nouser_subprocess); - g_test_add_func("/qdev/properties/dynamic/global/nouser", - test_dynamic_globalprop_nouser); + TEST_ADD_FUNC_SUBPROCESS("/qdev/properties/static/default", + test_static_prop); + + TEST_ADD_FUNC_SUBPROCESS("/qdev/properties/static/global", + test_static_globalprop); + + TEST_ADD_FUNC_SUBPROCESS("/qdev/properties/dynamic/global", + test_dynamic_globalprop); + + TEST_ADD_FUNC_SUBPROCESS("/qdev/properties/dynamic/global/nouser", + test_dynamic_globalprop_nouser); g_test_run();
Glib recently introduced a robust way to run tests in a subprocess, which is used in test-qdev-global-props. However, we would like to have the same tests run with older versions of glib, and the older fork-based mechanisms works well enough. This still requires: - bumping the minimum required version from 2.12 to 2.16. I suggest bumping to the currently required version for Windows, which is 2.20 (released March 2009). - disabling the test on Windows, since it does not have fork Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tests/Makefile | 2 +- tests/test-qdev-global-props.c | 63 +++++++++++++++++++++++++++--------------- 2 files changed, 41 insertions(+), 24 deletions(-)