GNOME Bugzilla – Bug 644941
glib-unix: New Unix-specific API
Last modified: 2011-04-27 20:02:10 UTC
GLib historically has been designed to be "mostly" portable; there are some functions only available on Unix like g_io_channel_unix_new(), but these are typically paired with obvious counterparts for Win32. However, as GLib is used not only by portable software, but components targeting Unix (or even just Linux), there are a few cases where it would be very convenient if GLib shipped built-in functionality. This initial patch is a basic wrapper around pipe2(), including fallbacks for older kernels. This pairs well with the existing g_spawn_*() API and its child_setup functionality. However, in the future, I want to add a signal() wrapper here, complete with proxying the signal to a mainloop. I have initial code for this, but doing it sanely (including factoring out gmain.c's private worker thread), is a complex task, and I don't want to block on that. See also gwin32.h for Win32 specific functionality.
Created attachment 183559 [details] [review] glib-unix: New Unix-specific API
Created attachment 183561 [details] [review] glib-unix: New Unix-specific API Add some docs, fix changes to gmain.c See also gwin32.h for Win32 specific functionality.
Created attachment 183562 [details] [review] glib-unix: New Unix-specific API And one more bugfix
+ g_set_error (error, + G_UNIX_ERROR, + 0, + "%s", strerror (errno)); g_set_error_literal, and g_strerror.
Created attachment 183628 [details] [review] glib-unix: New Unix-specific API GLib historically has been designed to be "mostly" portable; there are some functions only available on Unix like g_io_channel_unix_new(), but these are typically paired with obvious counterparts for Win32. However, as GLib is used not only by portable software, but components targeting Unix (or even just Linux), there are a few cases where it would be very convenient if GLib shipped built-in functionality. This initial patch is a basic wrapper around pipe2(), including fallbacks for older kernels. This pairs well with the existing g_spawn_*() API and its child_setup functionality. However, in the future, I want to add a signal() wrapper here, complete with proxying the signal to a mainloop. I have initial code for this, but doing it sanely (including factoring out gmain.c's private worker thread), is a complex task, and I don't want to block on that. See also gwin32.h for Win32 specific functionality.
Created attachment 183629 [details] [review] gmain: Prepare child watch handling for more generic signal handling In preparation for supporting more Unix signals such as SIGHUP, SIGTERM etc.,
Created attachment 183630 [details] [review] wip stubs to create a signal watch
(In reply to comment #4) > + g_set_error (error, > + G_UNIX_ERROR, > + 0, > + "%s", strerror (errno)); > > g_set_error_literal, and g_strerror. Thanks, fixed! I didn't know about g_strerror(). I also updated the docs around G_UNIX_ERROR.
David has such a signal source somewhere, too. Talk to him ?
(In reply to comment #9) > David has such a signal source somewhere, too. Talk to him ? http://cgit.freedesktop.org/PolicyKit/tree/src/polkitd/gposixsignal.c ? Only works with signalfd(), and I think if it's inside GLib, we can share the pipe and helper thread with the current GChildWatch source that we have anyways, and maintain compatibility with Linux < 2.6.22.
I actually like the idea of using signalfd quite a bit.
Created attachment 183799 [details] [review] glib-unix: New Unix-specific API No changes, reattaching for ordering.
Created attachment 183800 [details] [review] gmain: Prepare child watch handling for more generic signal handling This could probably be split off better from the next patch...but it's tricky.
Created attachment 183801 [details] [review] glib-unix: New API to watch some Unix signals This new API allows watching a few select Unix signals; looking through the list on my system, I didn't see anything else that I think it'd reasonable to watch. We build on the previous patch to make the child watch helper thread that existed on Unix handle these signals in the threaded case. In the non-threaded case, they're just global variables.
Review of attachment 183799 [details] [review]: Need to add a new section to glib-sections.txt for the new api, too. ::: glib/Makefile.am @@ +225,3 @@ +if OS_UNIX +glibinclude_HEADERS += glib-unix.h +endif Do we want to include glib-unix.h in glib.h ? ::: glib/glib-unix.c @@ +31,3 @@ + * SECTION:gunix + * @short_description: Unix-specific utilities and integration + * If we don't include glib-unix.h in glib.h, we need an @include: glib-unix.h here, and the docs should explicitly state that 'To use the UNIX-specific APIs, you must include <glib-unix.h>' @@ +34,3 @@ + * Most of GLib is intended to be mostly portable; in constrast, + * this set of functions is designed for programs which + * explicitly target Unix, or are using it to build higher In docs, we try to say 'UNIX' (all-caps) @@ +46,3 @@ + +static gboolean +_g_unix_set_error_from_errno (GError **error) If these are static, they don't need an _ prefix. ::: glib/glib-unix.h @@ +21,3 @@ +#define __G_UNIX_H__ + +#/* We need to include the Unix headers needed to use the APIs below, Stray #
Review of attachment 183800 [details] [review]: ::: glib/gmain.c @@ +3738,3 @@ + * that case, however, the context is still not destroyed + * and no poll can be active, otherwise the ref_count + * wouldn't be 0 */ Pet peeve: move the closing */ to the next line and make it line up @@ +4325,3 @@ + * and allow e.g. GDBus to use it instead of its own worker thread. + */ +static gpointer Why did we loose the G_GNUC_NORETURN here ?
Review of attachment 183801 [details] [review]: ::: glib/glib-unix.c @@ +138,3 @@ + * + * Create a #GSource that will be dispatched upon delivery of the Unix + * signal @signum. Currently only SIGHUP, SIGINT, and SIGTERM can be I wouldn't say 'currently', if we don't plan to support other signals, ever. I think SIGUSR1/2 would make sense to support, though. They are sometimes used to toggle e.g. debug output via kill; and it might be nice to do that via this mechanism. @@ +139,3 @@ + * Create a #GSource that will be dispatched upon delivery of the Unix + * signal @signum. Currently only SIGHUP, SIGINT, and SIGTERM can be + * monitored. Note that unlike the Unix default, all sources which s/Unix/UNIX/ for consistency, here. @@ +147,3 @@ + * g_main_loop_quit (). It is not safe to do any of this a regular + * Unix signal handler. + * Should have the same paragraph as other 'source_new' functions here: * The source will not initially be associated with any #GMainContext * and must be added to one with g_source_attach() before it will be * executed. @@ +151,3 @@ + */ +GSource * +g_unix_signal_create_watch (int signum) a bit odd to have a 'create_watch' function that returns a GSource. How about g_unix_signal_source_new ? that would go better with our other 'source_new' functions. @@ +161,3 @@ + * g_unix_signal_add_watch_full: + * @signum: Signal number + * @priority: the priority of the timeout source. Typically this will be in s/timeout// ::: glib/tests/unix.c @@ +100,3 @@ + g_main_loop_run (mainloop); + g_assert (!sigint_received); +} Would be good to test that removing the signal source works. Would be good to test other supported signals (and check that unsupported signals fail in the expected way) Would be good to repeat all tests in a multi-threaded scenario. Would be good to test interaction with sigprocmask and regular signal handlers.
(In reply to comment #15) > Review of attachment 183799 [details] [review]: > > Do we want to include glib-unix.h in glib.h ? That's a good question. Originally I had thought not, but glib.h does: #ifdef G_PLATFORM_WIN32 #include <glib/gwin32.h> #endif *however* to make the glib-unix.h API useful, we need to #include lots of random UNIX headers (I really don't see a value in G_SIGINT and G_FD_CLOEXEC). And if we're including all the UNIX headers, I'd rather not inflict that by default on every current user of <glib.h> if they happen to build on a UNIX platform. So let's keep the separate header? Fixed all your other comments.
(In reply to comment #16) >e the closing */ to the next line and make it line up > > @@ +4325,3 @@ > + * and allow e.g. GDBus to use it instead of its own worker thread. > + */ > +static gpointer > > Why did we loose the G_GNUC_NORETURN here ? I was doing "return NULL" in the basically impossible case that we get an error from read() on the pipe, but I just changed it to do g_usleep(G_USEC_PER_SEC) and continue.
(In reply to comment #17) > Review of attachment 183801 [details] [review]: > > ::: glib/glib-unix.c > @@ +138,3 @@ > + * > + * Create a #GSource that will be dispatched upon delivery of the Unix > + * signal @signum. Currently only SIGHUP, SIGINT, and SIGTERM can be > > I wouldn't say 'currently', if we don't plan to support other signals, ever. I > think SIGUSR1/2 would make sense to support, though. They are sometimes used to > toggle e.g. debug output via kill; and it might be nice to do that via this > mechanism. At least historically those two were used for Linuxthreads-internal purposes. I'm dubious about encouraging their use. One could use SIGINT for that purpose too. If someone chimes in on this thread and says they use them now and/or want to, we can add them. > Would be good to test other supported signals (and check that unsupported > signals fail in the expected way) Sort of gross to intentionally trigger a g_return_if_fail inside a test case =/ > Would be good to repeat all tests in a multi-threaded scenario. Yes. This is an unfortunate mess; because tests/glib is built *before* gthread =/ We could go to non-recursive make... > Would be good to test interaction with sigprocmask and regular signal handlers. I decided to just document sigprocmask()'s interaction as undefined. In practice, the semantics will depend on whether you link with gthread currently and thus whether the signal is handled in a separate thread or not.
Created attachment 186765 [details] [review] glib-unix: New Unix-specific API Updated for comments
Created attachment 186766 [details] [review] gmain: Prepare child watch handling for more generic signal handling Updated for comments
Created attachment 186767 [details] [review] glib-unix: New API to watch some Unix signals Updated for comments
So one thing *not* fixed by this series is the old race condition with g_child_watch if you're *not* linking to GThread: https://bugzilla.gnome.org/show_bug.cgi?id=398418 I think we could fix it as I described in https://bugzilla.gnome.org/show_bug.cgi?id=398418#c14 by writing the pid of the exiting process over the pipe (on Linux at least, you can get it by using SA_SIGINFO and siginfo_t).
(In reply to comment #20) > > Would be good to repeat all tests in a multi-threaded scenario. > > Yes. This is an unfortunate mess; because tests/glib is built *before* gthread > =/ glib/tests is built before gthread/, but tests/ comes after it (and, eg, has multithreaded gobject tests).
Created attachment 186772 [details] [review] glib-unix: New API to watch some Unix signals Now actually tested and working in a multithreaded scenario
Review of attachment 186765 [details] [review]: Looks good to me now
Review of attachment 186766 [details] [review]: ::: glib/gmain.c @@ +4324,3 @@ sigaction (SIGCHLD, &action, NULL); } Did you want to add G_GNUC_NORETURN back, now that we are again not returning ?
(In reply to comment #28) > Review of attachment 186766 [details] [review]: > > ::: glib/gmain.c > @@ +4324,3 @@ > sigaction (SIGCHLD, &action, NULL); > } > > > Did you want to add G_GNUC_NORETURN back, now that we are again not returning ? I did, see: +static gpointer unix_signal_helper_thread (gpointer data) G_GNUC_NORETURN;
Review of attachment 186772 [details] [review]: Other than these two, looks good to me. ::: glib/glib-unix.c @@ +142,3 @@ + * Create a #GSource that will be dispatched upon delivery of the Unix + * signal @signum. Currently only SIGHUP, SIGINT, and SIGTERM can be + * monitored. Note that unlike the Unix default, all sources which Some Unix left here... @@ +157,3 @@ + * functions like sigprocmask() is not defined. + * + * NOTE: For reliable behavior, if your program links to gthread For notes, I tend to use <note></note>, so we get a nice colorful box on the web...
Attachment 186765 [details] pushed as 0ff211f - glib-unix: New Unix-specific API Attachment 186766 [details] pushed as 920899d - gmain: Prepare child watch handling for more generic signal handling Attachment 186772 [details] pushed as 549d895 - glib-unix: New API to watch some Unix signals