[v2] glib-compat: work around g_test_message bug with subprocess tests

[Qemu-devel] [PATCH v2] glib-compat: work around g_test_message bug with subprocess tests

Posted by Paolo Bonzini 7 years, 2 months ago

Subprocesses are created by glib without leaving the file descriptors
open.  Therefore, g_test_message (and assertion failures, but those
trigger when things are going bad anyway) will think that it is writing
to the log file descriptor, but while actually stomping on the QMP
file descriptor or similar.  This causes spurious failures, which are
as nice to debug as the reader can imagine.  While I have opened a
pull request on GLib, this will probably take a while to propagate
to distros.

I found this while working on qgraph, but the fix is generic.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1543343726-53531-1-git-send-email-pbonzini@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/glib-compat.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index fdf95a255d..f675c7328f 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -113,4 +113,12 @@ gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
 
 #pragma GCC diagnostic pop
 
+/* See https://gitlab.gnome.org/GNOME/glib/merge_requests/501 */
+#define g_test_message(...)                                     \
+    do {                                                        \
+        if (!g_test_subprocess()) {                             \
+            g_test_message(__VA_ARGS__);                        \
+        }                                                       \
+    } while (0)
+
 #endif
-- 
2.19.1

Re: [Qemu-devel] [PATCH v2] glib-compat: work around g_test_message bug with subprocess tests

Posted by Paolo Bonzini 7 years, 2 months ago

On 29/11/18 19:00, Paolo Bonzini wrote:
> Subprocesses are created by glib without leaving the file descriptors
> open.  Therefore, g_test_message (and assertion failures, but those
> trigger when things are going bad anyway) will think that it is writing
> to the log file descriptor, but while actually stomping on the QMP
> file descriptor or similar.  This causes spurious failures, which are
> as nice to debug as the reader can imagine.  While I have opened a
> pull request on GLib, this will probably take a while to propagate
> to distros.
> 
> I found this while working on qgraph, but the fix is generic.

This is not a problem anymore if we switch from gtester to tap.  Do we
want it fixed anyway or not?

Paolo

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <1543343726-53531-1-git-send-email-pbonzini@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/glib-compat.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index fdf95a255d..f675c7328f 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -113,4 +113,12 @@ gint g_poll_fixed(GPollFD *fds, guint nfds, gint timeout);
>  
>  #pragma GCC diagnostic pop
>  
> +/* See https://gitlab.gnome.org/GNOME/glib/merge_requests/501 */
> +#define g_test_message(...)                                     \
> +    do {                                                        \
> +        if (!g_test_subprocess()) {                             \
> +            g_test_message(__VA_ARGS__);                        \
> +        }                                                       \
> +    } while (0)
> +
>  #endif
>

Re: [Qemu-devel] [PATCH v2] glib-compat: work around g_test_message bug with subprocess tests

Posted by Eric Blake 7 years, 2 months ago

On 11/29/18 6:08 PM, Paolo Bonzini wrote:
> On 29/11/18 19:00, Paolo Bonzini wrote:
>> Subprocesses are created by glib without leaving the file descriptors
>> open.  Therefore, g_test_message (and assertion failures, but those
>> trigger when things are going bad anyway) will think that it is writing
>> to the log file descriptor, but while actually stomping on the QMP
>> file descriptor or similar.  This causes spurious failures, which are
>> as nice to debug as the reader can imagine.  While I have opened a
>> pull request on GLib, this will probably take a while to propagate
>> to distros.
>>
>> I found this while working on qgraph, but the fix is generic.
> 
> This is not a problem anymore if we switch from gtester to tap.  Do we
> want it fixed anyway or not?

If there is no -rc4, I'm fine if we drop this patch in favor of your 
other patches for TAP. But if we have -rc4, this may still be useful for 
inclusion in 3.1 for testsuite robustness.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2] glib-compat: work around g_test_message bug with subprocess tests

Posted by Daniel P. Berrangé 7 years, 2 months ago

On Fri, Nov 30, 2018 at 01:08:29AM +0100, Paolo Bonzini wrote:
> On 29/11/18 19:00, Paolo Bonzini wrote:
> > Subprocesses are created by glib without leaving the file descriptors
> > open.  Therefore, g_test_message (and assertion failures, but those
> > trigger when things are going bad anyway) will think that it is writing
> > to the log file descriptor, but while actually stomping on the QMP
> > file descriptor or similar.  This causes spurious failures, which are
> > as nice to debug as the reader can imagine.  While I have opened a
> > pull request on GLib, this will probably take a while to propagate
> > to distros.
> > 
> > I found this while working on qgraph, but the fix is generic.
> 
> This is not a problem anymore if we switch from gtester to tap.  Do we
> want it fixed anyway or not?

IMHO the change to tap is not really something  we shoudl be doing
at this point in the release cycle. So I'd favour taking this fix
for 3.1, even if it becomes redundant when we switch to tap in 4.0


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 :|

Re: [Qemu-devel] [PATCH v2] glib-compat: work around g_test_message bug with subprocess tests

Posted by Paolo Bonzini 7 years, 2 months ago

On 30/11/18 10:58, Daniel P. Berrangé wrote:
> On Fri, Nov 30, 2018 at 01:08:29AM +0100, Paolo Bonzini wrote:
>> On 29/11/18 19:00, Paolo Bonzini wrote:
>>> Subprocesses are created by glib without leaving the file descriptors
>>> open.  Therefore, g_test_message (and assertion failures, but those
>>> trigger when things are going bad anyway) will think that it is writing
>>> to the log file descriptor, but while actually stomping on the QMP
>>> file descriptor or similar.  This causes spurious failures, which are
>>> as nice to debug as the reader can imagine.  While I have opened a
>>> pull request on GLib, this will probably take a while to propagate
>>> to distros.
>>>
>>> I found this while working on qgraph, but the fix is generic.
>>
>> This is not a problem anymore if we switch from gtester to tap.  Do we
>> want it fixed anyway or not?
> 
> IMHO the change to tap is not really something  we shoudl be doing
> at this point in the release cycle.

Of course---I wasn't suggesting that, the TAP switch is for 4.0.
However, this fix is not needed for 3.1 as far as I know, and we're
pretty late in 3.1's rc period too.

Paolo

> So I'd favour taking this fix
> for 3.1, even if it becomes redundant when we switch to tap in 4.0