qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h
@ 2014-02-03 13:31 Stefan Hajnoczi
  2014-02-03 13:31 ` [Qemu-devel] [PATCH 1/3] glib: move g_poll() replacement " Stefan Hajnoczi
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-02-03 13:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, Anthony Liguori

glib has deprecated APIs like GStaticMutex, g_thread_create(), and others.  In
QEMU support both old and new APIs since using deprecated APIs would flood us
with warnings but legacy distros must continue to build the QEMU source code.

This patch series reduces ifdefs by moving glib compat functions into
glib-compat.h, where they can be reused.

There are two strategies for compat functions:

1. Implement the new API using the deprecated API.  This compat function is
   used when building on a legacy host.  Sometimes the API semantics are so
   different that this option is not feasible.

2. Add a new wrapper API that maps to the deprecated API.  The wrapper is not
   marked deprecated so it works as a drop-in replacement but is implemented
   using the new API where possible.

Stefan Hajnoczi (3):
  glib: move g_poll() replacement into glib-compat.h
  glib: add g_thread_new() compat function
  glib: add compat wrapper for GStaticMutex

 coroutine-gthread.c   | 26 ++++++++++----------------
 include/glib-compat.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/qemu-common.h | 12 ------------
 trace/simple.c        | 31 ++++++++++---------------------
 4 files changed, 64 insertions(+), 49 deletions(-)

-- 
1.8.5.3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 1/3] glib: move g_poll() replacement into glib-compat.h
  2014-02-03 13:31 [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
@ 2014-02-03 13:31 ` Stefan Hajnoczi
  2014-02-03 13:31 ` [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function Stefan Hajnoczi
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-02-03 13:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, Anthony Liguori

We have a dedicated header file for wrappers to smooth over glib version
differences.  Move the g_poll() definition into glib-compat.h for
consistency.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/glib-compat.h | 12 ++++++++++++
 include/qemu-common.h | 12 ------------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 8aa77af..8d25900 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -24,4 +24,16 @@ static inline guint g_timeout_add_seconds(guint interval, GSourceFunc function,
 }
 #endif
 
+#if !GLIB_CHECK_VERSION(2, 20, 0)
+/*
+ * Glib before 2.20.0 doesn't implement g_poll, so wrap it to compile properly
+ * on older systems.
+ */
+static inline gint g_poll(GPollFD *fds, guint nfds, gint timeout)
+{
+    GMainContext *ctx = g_main_context_default();
+    return g_main_context_get_poll_func(ctx)(fds, nfds, timeout);
+}
+#endif
+
 #endif
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 5054836..e0e03cc 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -124,18 +124,6 @@ int qemu_main(int argc, char **argv, char **envp);
 void qemu_get_timedate(struct tm *tm, int offset);
 int qemu_timedate_diff(struct tm *tm);
 
-#if !GLIB_CHECK_VERSION(2, 20, 0)
-/*
- * Glib before 2.20.0 doesn't implement g_poll, so wrap it to compile properly
- * on older systems.
- */
-static inline gint g_poll(GPollFD *fds, guint nfds, gint timeout)
-{
-    GMainContext *ctx = g_main_context_default();
-    return g_main_context_get_poll_func(ctx)(fds, nfds, timeout);
-}
-#endif
-
 /**
  * is_help_option:
  * @s: string to test
-- 
1.8.5.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function
  2014-02-03 13:31 [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
  2014-02-03 13:31 ` [Qemu-devel] [PATCH 1/3] glib: move g_poll() replacement " Stefan Hajnoczi
@ 2014-02-03 13:31 ` Stefan Hajnoczi
  2014-02-03 13:31 ` [Qemu-devel] [PATCH 3/3] glib: add compat wrapper for GStaticMutex Stefan Hajnoczi
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-02-03 13:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, Anthony Liguori

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 3/3] glib: add compat wrapper for GStaticMutex
  2014-02-03 13:31 [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
  2014-02-03 13:31 ` [Qemu-devel] [PATCH 1/3] glib: move g_poll() replacement " Stefan Hajnoczi
  2014-02-03 13:31 ` [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function Stefan Hajnoczi
@ 2014-02-03 13:31 ` Stefan Hajnoczi
  2014-02-14 12:31 ` [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
  2014-05-02  9:35 ` Stefan Hajnoczi
  4 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-02-03 13:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, Anthony Liguori

Avoid duplicating ifdefs for the deprecated GStaticMutex API by
introducing compat_g_static_mutex_*() in glib-compat.h.

GStaticMutex users should use the compat API to avoid dealing with
deprecation.

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

diff --git a/coroutine-gthread.c b/coroutine-gthread.c
index 695c113..28272f8 100644
--- a/coroutine-gthread.c
+++ b/coroutine-gthread.c
@@ -30,7 +30,7 @@ typedef struct {
     CoroutineAction action;
 } CoroutineGThread;
 
-static GStaticMutex coroutine_lock = G_STATIC_MUTEX_INIT;
+DEFINE_COMPAT_G_STATIC_MUTEX(coroutine_lock);
 
 /* GLib 2.31 and beyond deprecated various parts of the thread API,
  * but the new interfaces are not available in older GLib versions
@@ -123,15 +123,16 @@ static void __attribute__((constructor)) coroutine_init(void)
 static void coroutine_wait_runnable_locked(CoroutineGThread *co)
 {
     while (!co->runnable) {
-        g_cond_wait(coroutine_cond, g_static_mutex_get_mutex(&coroutine_lock));
+        g_cond_wait(coroutine_cond,
+                    compat_g_static_mutex_get_mutex(&coroutine_lock));
     }
 }
 
 static void coroutine_wait_runnable(CoroutineGThread *co)
 {
-    g_static_mutex_lock(&coroutine_lock);
+    compat_g_static_mutex_lock(&coroutine_lock);
     coroutine_wait_runnable_locked(co);
-    g_static_mutex_unlock(&coroutine_lock);
+    compat_g_static_mutex_unlock(&coroutine_lock);
 }
 
 static gpointer coroutine_thread(gpointer opaque)
@@ -173,7 +174,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_,
     CoroutineGThread *from = DO_UPCAST(CoroutineGThread, base, from_);
     CoroutineGThread *to = DO_UPCAST(CoroutineGThread, base, to_);
 
-    g_static_mutex_lock(&coroutine_lock);
+    compat_g_static_mutex_lock(&coroutine_lock);
     from->runnable = false;
     from->action = action;
     to->runnable = true;
@@ -183,7 +184,7 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_,
     if (action != COROUTINE_TERMINATE) {
         coroutine_wait_runnable_locked(from);
     }
-    g_static_mutex_unlock(&coroutine_lock);
+    compat_g_static_mutex_unlock(&coroutine_lock);
     return from->action;
 }
 
diff --git a/include/glib-compat.h b/include/glib-compat.h
index ea965df..48fa95c 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -49,4 +49,23 @@ static inline GThread *g_thread_new(const gchar *unused,
 }
 #endif
 
+/* GStaticMutex was deprecated in 2.32.0.  Use the compat_g_static_mutex_*()
+ * wrappers to hide the GStaticMutex/GMutex distinction.
+ */
+#if GLIB_CHECK_VERSION(2, 32, 0)
+#define DEFINE_COMPAT_G_STATIC_MUTEX(name) \
+static GMutex name
+
+#define compat_g_static_mutex_lock(m) g_mutex_lock(m)
+#define compat_g_static_mutex_unlock(m) g_mutex_unlock(m)
+#define compat_g_static_mutex_get_mutex(m) (m)
+#else
+#define DEFINE_COMPAT_G_STATIC_MUTEX(name) \
+static GStaticMutex name = G_STATIC_MUTEX_INIT
+
+#define compat_g_static_mutex_lock(m) g_static_mutex_lock(m)
+#define compat_g_static_mutex_unlock(m) g_static_mutex_unlock(m)
+#define compat_g_static_mutex_get_mutex(m) g_static_mutex_get_mutex(m)
+#endif
+
 #endif
diff --git a/trace/simple.c b/trace/simple.c
index 8e83e59..dc58afe 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -41,17 +41,7 @@
  * Trace records are written out by a dedicated thread.  The thread waits for
  * records to become available, writes them out, and then waits again.
  */
-#if GLIB_CHECK_VERSION(2, 32, 0)
-static GMutex trace_lock;
-#define lock_trace_lock() g_mutex_lock(&trace_lock)
-#define unlock_trace_lock() g_mutex_unlock(&trace_lock)
-#define get_trace_lock_mutex() (&trace_lock)
-#else
-static GStaticMutex trace_lock = G_STATIC_MUTEX_INIT;
-#define lock_trace_lock() g_static_mutex_lock(&trace_lock)
-#define unlock_trace_lock() g_static_mutex_unlock(&trace_lock)
-#define get_trace_lock_mutex() g_static_mutex_get_mutex(&trace_lock)
-#endif
+DEFINE_COMPAT_G_STATIC_MUTEX(trace_lock);
 
 /* g_cond_new() was deprecated in glib 2.31 but we still need to support it */
 #if GLIB_CHECK_VERSION(2, 31, 0)
@@ -151,26 +141,28 @@ static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
  */
 static void flush_trace_file(bool wait)
 {
-    lock_trace_lock();
+    compat_g_static_mutex_lock(&trace_lock);
     trace_available = true;
     g_cond_signal(trace_available_cond);
 
     if (wait) {
-        g_cond_wait(trace_empty_cond, get_trace_lock_mutex());
+        g_cond_wait(trace_empty_cond,
+                    compat_g_static_mutex_get_mutex(&trace_lock));
     }
 
-    unlock_trace_lock();
+    compat_g_static_mutex_unlock(&trace_lock);
 }
 
 static void wait_for_trace_records_available(void)
 {
-    lock_trace_lock();
+    compat_g_static_mutex_lock(&trace_lock);
     while (!(trace_available && trace_writeout_enabled)) {
         g_cond_signal(trace_empty_cond);
-        g_cond_wait(trace_available_cond, get_trace_lock_mutex());
+        g_cond_wait(trace_available_cond,
+                    compat_g_static_mutex_get_mutex(&trace_lock));
     }
     trace_available = false;
-    unlock_trace_lock();
+    compat_g_static_mutex_unlock(&trace_lock);
 }
 
 static gpointer writeout_thread(gpointer opaque)
-- 
1.8.5.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h
  2014-02-03 13:31 [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
                   ` (2 preceding siblings ...)
  2014-02-03 13:31 ` [Qemu-devel] [PATCH 3/3] glib: add compat wrapper for GStaticMutex Stefan Hajnoczi
@ 2014-02-14 12:31 ` Stefan Hajnoczi
  2014-05-02  9:35 ` Stefan Hajnoczi
  4 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-02-14 12:31 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: Peter Maydell, qemu-devel, Anthony Liguori

On Mon, Feb 03, 2014 at 02:31:47PM +0100, Stefan Hajnoczi wrote:
> glib has deprecated APIs like GStaticMutex, g_thread_create(), and others.  In
> QEMU support both old and new APIs since using deprecated APIs would flood us
> with warnings but legacy distros must continue to build the QEMU source code.
> 
> This patch series reduces ifdefs by moving glib compat functions into
> glib-compat.h, where they can be reused.
> 
> There are two strategies for compat functions:
> 
> 1. Implement the new API using the deprecated API.  This compat function is
>    used when building on a legacy host.  Sometimes the API semantics are so
>    different that this option is not feasible.
> 
> 2. Add a new wrapper API that maps to the deprecated API.  The wrapper is not
>    marked deprecated so it works as a drop-in replacement but is implemented
>    using the new API where possible.
> 
> Stefan Hajnoczi (3):
>   glib: move g_poll() replacement into glib-compat.h
>   glib: add g_thread_new() compat function
>   glib: add compat wrapper for GStaticMutex
> 
>  coroutine-gthread.c   | 26 ++++++++++----------------
>  include/glib-compat.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/qemu-common.h | 12 ------------
>  trace/simple.c        | 31 ++++++++++---------------------
>  4 files changed, 64 insertions(+), 49 deletions(-)

Ping?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h
  2014-02-03 13:31 [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
                   ` (3 preceding siblings ...)
  2014-02-14 12:31 ` [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
@ 2014-05-02  9:35 ` Stefan Hajnoczi
  4 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2014-05-02  9:35 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Peter Maydell, Michael Tokarev, qemu-devel, Anthony Liguori

On Mon, Feb 03, 2014 at 02:31:47PM +0100, Stefan Hajnoczi wrote:
> glib has deprecated APIs like GStaticMutex, g_thread_create(), and others.  In
> QEMU support both old and new APIs since using deprecated APIs would flood us
> with warnings but legacy distros must continue to build the QEMU source code.
> 
> This patch series reduces ifdefs by moving glib compat functions into
> glib-compat.h, where they can be reused.
> 
> There are two strategies for compat functions:
> 
> 1. Implement the new API using the deprecated API.  This compat function is
>    used when building on a legacy host.  Sometimes the API semantics are so
>    different that this option is not feasible.
> 
> 2. Add a new wrapper API that maps to the deprecated API.  The wrapper is not
>    marked deprecated so it works as a drop-in replacement but is implemented
>    using the new API where possible.
> 
> Stefan Hajnoczi (3):
>   glib: move g_poll() replacement into glib-compat.h
>   glib: add g_thread_new() compat function
>   glib: add compat wrapper for GStaticMutex
> 
>  coroutine-gthread.c   | 26 ++++++++++----------------
>  include/glib-compat.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/qemu-common.h | 12 ------------
>  trace/simple.c        | 31 ++++++++++---------------------
>  4 files changed, 64 insertions(+), 49 deletions(-)

Ping^2?

Also CCed mjt because he posted a related series.

Stefan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function
@ 2014-05-02 10:52 Michael Tokarev
  2014-05-02 11:01 ` Daniel P. Berrange
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Tokarev @ 2014-05-02 10:52 UTC (permalink / raw)
  To: stefanha; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function
  2014-05-02 10:52 [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function Michael Tokarev
@ 2014-05-02 11:01 ` Daniel P. Berrange
  2014-05-02 11:08   ` Peter Maydell
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel P. Berrange @ 2014-05-02 11:01 UTC (permalink / raw)
  To: Michael Tokarev; +Cc: qemu-devel, stefanha

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function
  2014-05-02 11:01 ` Daniel P. Berrange
@ 2014-05-02 11:08   ` Peter Maydell
  2014-05-02 11:16     ` Michael Tokarev
  2014-05-02 12:30     ` Michael Tokarev
  0 siblings, 2 replies; 11+ messages in thread
From: Peter Maydell @ 2014-05-02 11:08 UTC (permalink / raw)
  To: Daniel P. Berrange; +Cc: Michael Tokarev, qemu-devel, Stefan Hajnoczi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function
  2014-05-02 11:08   ` Peter Maydell
@ 2014-05-02 11:16     ` Michael Tokarev
  2014-05-02 12:30     ` Michael Tokarev
  1 sibling, 0 replies; 11+ messages in thread
From: Michael Tokarev @ 2014-05-02 11:16 UTC (permalink / raw)
  To: Peter Maydell, Daniel P. Berrange; +Cc: qemu-devel, Stefan Hajnoczi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function
  2014-05-02 11:08   ` Peter Maydell
  2014-05-02 11:16     ` Michael Tokarev
@ 2014-05-02 12:30     ` Michael Tokarev
  1 sibling, 0 replies; 11+ messages in thread
From: Michael Tokarev @ 2014-05-02 12:30 UTC (permalink / raw)
  To: Peter Maydell, Daniel P. Berrange; +Cc: qemu-devel, Stefan Hajnoczi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-05-02 12:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-03 13:31 [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
2014-02-03 13:31 ` [Qemu-devel] [PATCH 1/3] glib: move g_poll() replacement " Stefan Hajnoczi
2014-02-03 13:31 ` [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function Stefan Hajnoczi
2014-02-03 13:31 ` [Qemu-devel] [PATCH 3/3] glib: add compat wrapper for GStaticMutex Stefan Hajnoczi
2014-02-14 12:31 ` [Qemu-devel] [PATCH 0/3] glib: move compat functions into glib-compat.h Stefan Hajnoczi
2014-05-02  9:35 ` Stefan Hajnoczi
  -- strict thread matches above, loose matches on Subject: below --
2014-05-02 10:52 [Qemu-devel] [PATCH 2/3] glib: add g_thread_new() compat function Michael Tokarev
2014-05-02 11:01 ` Daniel P. Berrange
2014-05-02 11:08   ` Peter Maydell
2014-05-02 11:16     ` Michael Tokarev
2014-05-02 12:30     ` Michael Tokarev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).