[Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function

[Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function

[Michael Tokarev]

Stefan Hajnoczi:
> Implement g_thread_new() in terms of the deprecated g_thread_create().
> The API was changed in glib 2.31.0.
>
> The compat function allows us to write modern code and avoid ifdefs.

ACK.  With one small nit:

[]
> +#if !GLIB_CHECK_VERSION(2, 31, 0)
> +static inline GThread *g_thread_new(const gchar *unused,
> +                                    GThreadFunc func,
> +                                    gpointer data)
> +{
> +    GThread *thread = g_thread_create(func, data, TRUE, NULL);
> +    if (!thread) {
> +        g_error("g_thread_create failed");
> +    }
> +    return thread;
> +}
> +#endif

About g_error():

"This function will result in a core dump; don't use it for errors you expect.
 Using this function indicates a bug in your program, i.e. an assertion failure."

I'm not sure if this is like an assertion failure, probably yes.  But we should
not, I think, dump core in this situation.  Is there a glib function that does
(fatal) error reporting but does NOT dump core?

In my attempt to do this, I completely ignored errors:

 https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg04551.html

(search for g_thread_new() in glib-compat.h).  This is, obviously, not right,
but that's really because I was too lazy to actually find the right function
to do so.  Maybe just using some fprintf(stderr) + abort() - from classic
C library instead of glib - will do.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function

[Daniel P. Berrange]

On Fri, May 02, 2014 at 02:52:23PM +0400, Michael Tokarev wrote:
> Stefan Hajnoczi:
> > Implement g_thread_new() in terms of the deprecated g_thread_create().
> > The API was changed in glib 2.31.0.
> >
> > The compat function allows us to write modern code and avoid ifdefs.
> 
> ACK.  With one small nit:
> 
> []
> > +#if !GLIB_CHECK_VERSION(2, 31, 0)
> > +static inline GThread *g_thread_new(const gchar *unused,
> > +                                    GThreadFunc func,
> > +                                    gpointer data)
> > +{
> > +    GThread *thread = g_thread_create(func, data, TRUE, NULL);
> > +    if (!thread) {
> > +        g_error("g_thread_create failed");
> > +    }
> > +    return thread;
> > +}
> > +#endif
> 
> About g_error():
> 
> "This function will result in a core dump; don't use it for errors you expect.
>  Using this function indicates a bug in your program, i.e. an assertion failure."
> 
> I'm not sure if this is like an assertion failure, probably yes.  But we should
> not, I think, dump core in this situation.  Is there a glib function that does
> (fatal) error reporting but does NOT dump core?

I think that's what  g_critical is intended for. It is more severe than
g_warning, but won't exit or abort the process.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function

[Peter Maydell]

On 2 May 2014 12:01, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Fri, May 02, 2014 at 02:52:23PM +0400, Michael Tokarev wrote:
>> Stefan Hajnoczi:
>> > +#if !GLIB_CHECK_VERSION(2, 31, 0)
>> > +static inline GThread *g_thread_new(const gchar *unused,
>> > +                                    GThreadFunc func,
>> > +                                    gpointer data)
>> > +{
>> > +    GThread *thread = g_thread_create(func, data, TRUE, NULL);
>> > +    if (!thread) {
>> > +        g_error("g_thread_create failed");
>> > +    }
>> > +    return thread;
>> > +}
>> > +#endif
>>
>> About g_error():
>>
>> "This function will result in a core dump; don't use it for errors you expect.
>>  Using this function indicates a bug in your program, i.e. an assertion failure."
>>
>> I'm not sure if this is like an assertion failure, probably yes.  But we should
>> not, I think, dump core in this situation.  Is there a glib function that does
>> (fatal) error reporting but does NOT dump core?
>
> I think that's what  g_critical is intended for. It is more severe than
> g_warning, but won't exit or abort the process.

I'm not convinced we should be emitting any kind of
warning message here anyway -- surely it's up to the
caller to handle thread creation failure? The glib
warning/error functions presumably print to stderr,
which has all the usual issues with possibly messing
up guest output.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function

[Michael Tokarev]

02.05.2014 15:08, Peter Maydell wrote:

>>> Stefan Hajnoczi:
>>>> +#if !GLIB_CHECK_VERSION(2, 31, 0)
>>>> +static inline GThread *g_thread_new(const gchar *unused,
>>>> +                                    GThreadFunc func,
>>>> +                                    gpointer data)
>>>> +{
>>>> +    GThread *thread = g_thread_create(func, data, TRUE, NULL);
>>>> +    if (!thread) {
>>>> +        g_error("g_thread_create failed");
>>>> +    }
>>>> +    return thread;
>>>> +}
>>>> +#endif
>>>
>>> About g_error():
>>>
>>> "This function will result in a core dump; don't use it for errors you expect.
>>>  Using this function indicates a bug in your program, i.e. an assertion failure."

> I'm not convinced we should be emitting any kind of
> warning message here anyway -- surely it's up to the
> caller to handle thread creation failure? The glib
> warning/error functions presumably print to stderr,
> which has all the usual issues with possibly messing
> up guest output.

Note that QemuThread &Co has exactly the same issue.

void qemu_mutex_init(QemuMutex *mutex)
{
...
    if (err)
        error_exit(err, __func__);
}

and so on in all util/qemu-thread-{posix,win32).c

But yes, you're right, at least coroutine-gthread.c
appears to try to handle errors by its own.

So this is what's missing from my patchset for libcacard -
since this one replaces qemu thread primitives (which
aborts on error) with glib thread primitives (which
return error condition instead).

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function

[Michael Tokarev]

02.05.2014 15:08, Peter Maydell wrote:
> On 2 May 2014 12:01, Daniel P. Berrange <berrange@redhat.com> wrote:
>> On Fri, May 02, 2014 at 02:52:23PM +0400, Michael Tokarev wrote:
>>> Stefan Hajnoczi:
>>>> +#if !GLIB_CHECK_VERSION(2, 31, 0)
>>>> +static inline GThread *g_thread_new(const gchar *unused,
>>>> +                                    GThreadFunc func,
>>>> +                                    gpointer data)
>>>> +{
>>>> +    GThread *thread = g_thread_create(func, data, TRUE, NULL);
>>>> +    if (!thread) {
>>>> +        g_error("g_thread_create failed");
>>>> +    }
>>>> +    return thread;
>>>> +}
>>>> +#endif
>>>
>>> About g_error():
>>>
>>> "This function will result in a core dump; don't use it for errors you expect.
>>>  Using this function indicates a bug in your program, i.e. an assertion failure."
>>>
>>> I'm not sure if this is like an assertion failure, probably yes.  But we should
>>> not, I think, dump core in this situation.  Is there a glib function that does
>>> (fatal) error reporting but does NOT dump core?
>>
>> I think that's what  g_critical is intended for. It is more severe than
>> g_warning, but won't exit or abort the process.
> 
> I'm not convinced we should be emitting any kind of
> warning message here anyway -- surely it's up to the
> caller to handle thread creation failure? The glib
> warning/error functions presumably print to stderr,
> which has all the usual issues with possibly messing
> up guest output.

Actually the whole point is moot.  Here's what g_thread_new() does (in 2.31+):

GThread *
g_thread_new (const gchar *name,
              GThreadFunc  func,
              gpointer     data)
{
  GError *error = NULL;
  GThread *thread;

  thread = g_thread_new_internal (name, g_thread_proxy, func, data, 0, &error);

  if G_UNLIKELY (thread == NULL)
    g_error ("creating thread '%s': %s", name ? name : "", error->message);

  return thread;
}

So that's what we should use in our g_thread_new() impl,
and this is what Stefan used, in a slightly less complex way.

I'll add this to my wrapper too.

Thanks,

/mjt

[Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function

[Stefan Hajnoczi]

Implement g_thread_new() in terms of the deprecated g_thread_create().
The API was changed in glib 2.31.0.

The compat function allows us to write modern code and avoid ifdefs.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 coroutine-gthread.c   | 13 +++----------
 include/glib-compat.h | 13 +++++++++++++
 trace/simple.c        |  5 +----
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/coroutine-gthread.c b/coroutine-gthread.c
index d3e5b99..695c113 100644
--- a/coroutine-gthread.c
+++ b/coroutine-gthread.c
@@ -76,11 +76,6 @@ static inline void set_coroutine_key(CoroutineGThread *co,
     g_private_replace(&coroutine_key, co);
 }
 
-static inline GThread *create_thread(GThreadFunc func, gpointer data)
-{
-    return g_thread_new("coroutine", func, data);
-}
-
 #else
 
 /* Handle older GLib versions */
@@ -104,15 +99,13 @@ static inline void set_coroutine_key(CoroutineGThread *co,
                          free_on_thread_exit ? (GDestroyNotify)g_free : NULL);
 }
 
+#endif
+
 static inline GThread *create_thread(GThreadFunc func, gpointer data)
 {
-    return g_thread_create_full(func, data, 0, TRUE, TRUE,
-                                G_THREAD_PRIORITY_NORMAL, NULL);
+    return g_thread_new("coroutine", func, data);
 }
 
-#endif
-
-
 static void __attribute__((constructor)) coroutine_init(void)
 {
     if (!g_thread_supported()) {
diff --git a/include/glib-compat.h b/include/glib-compat.h
index 8d25900..ea965df 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -36,4 +36,17 @@ static inline gint g_poll(GPollFD *fds, guint nfds, gint timeout)
 }
 #endif
 
+#if !GLIB_CHECK_VERSION(2, 31, 0)
+static inline GThread *g_thread_new(const gchar *unused,
+                                    GThreadFunc func,
+                                    gpointer data)
+{
+    GThread *thread = g_thread_create(func, data, TRUE, NULL);
+    if (!thread) {
+        g_error("g_thread_create failed");
+    }
+    return thread;
+}
+#endif
+
 #endif
diff --git a/trace/simple.c b/trace/simple.c
index 57572c4..8e83e59 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -17,6 +17,7 @@
 #include <pthread.h>
 #endif
 #include "qemu/timer.h"
+#include "glib-compat.h"
 #include "trace.h"
 #include "trace/control.h"
 #include "trace/simple.h"
@@ -397,11 +398,7 @@ static GThread *trace_thread_create(GThreadFunc fn)
     pthread_sigmask(SIG_SETMASK, &set, &oldset);
 #endif
 
-#if GLIB_CHECK_VERSION(2, 31, 0)
     thread = g_thread_new("trace-thread", fn, NULL);
-#else
-    thread = g_thread_create(fn, NULL, FALSE, NULL);
-#endif
 
 #ifndef _WIN32
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
-- 
1.8.5.3