[Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Paolo Bonzini]

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(-)

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();
 
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Peter Maydell]

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

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Paolo Bonzini]

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

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Peter Maydell]

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

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Paolo Bonzini]

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

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Michael S. Tsirkin]

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?

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Michael S. Tsirkin]

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?

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Peter Maydell]

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

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Michael S. Tsirkin]

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.

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Peter Maydell]

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

Re: [Qemu-devel] [RFC PATCH] tests: use g_test_trap_fork for glib between 2.16 and 2.38

[Michael S. Tsirkin]

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?

-- 
MST