GNOME Bugzilla – Bug 693481
gst-launch: Use signalfd() to handle keyboard interruption
Last modified: 2013-02-14 09:12:03 UTC
Created attachment 235588 [details] [review] Proposed patch The current implementation uses a traditional signal handler and a 250ms timeout callback in the event loop. Using signalfd() and polling a file descriptor in the GMainLoop is a more elegant solutions. The signal handler with this approach can send a message to the bus directly rather than set a flag. The old approach is kept for backwards compatibility as signalfd() might not be fully supported on all systems. Questions: * Is preserving old method required?
I don't understand the logic behind waiting_eos. By default it's FALSE so if a signal handler is invoked, it'll deregister itself (sigint_restore()). If -e option is used, setting waiting_eos to TRUE makes little sense as the signal handler is already restored to the default one and will not be invoked any more. In the proposed patch I set waiting_eos to TRUE by default as this is what we naturally expect to happen. I feel uneasy about it though as there is of course a number of error conditions and at some point for the sake of diligence SIGINT handler should be deregistered once we know there'll be no bus activity expected any more.
Actually, I think maybe we should rip out all of this code and use g_unix_signal_add() instead, which was apparently added in GLib 2.30: http://developer.gnome.org/glib/2.30/glib-UNIX-specific-utilities-and-integration.html#g-unix-signal-add It does not use signalfd yet, but that would be an even better place to add it then.
Tempting. I'll have a look.
Created attachment 235842 [details] [review] A new patch that rips out raw signal handling and uses g_source_set_callback() instead Indeed, g_source_set_callback() seems like an elegant approach. Although it internally uses a traditional signal handler, it does the dirty job of dispatching GSource handler from the GMainContext in a safe manner. It'd be nice to do some research and re-plumbing of this area in GLib to use signalfd() but from the GStreamer perspective it's an implementation detail and not a concern. Please have a look and let me know what you think. Cheers, Kris
(In reply to comment #4) > Indeed, g_source_set_callback() seems like an elegant approach. I meant g_unix_signal_add () rather than g_source_set_callback().
Review of attachment 235842 [details] [review]: ::: tools/gst-launch.c @@ +38,3 @@ #endif #ifndef DISABLE_FAULT_HANDLER +#include <glib-unix.h> Will this block only be used on Unix-like platforms? @@ +517,3 @@ + gst_message_new_application (GST_OBJECT (pipeline), + gst_structure_new ("GstLaunchInterrupt", + "message", G_TYPE_STRING, "Pipeline interrupted", NULL))); As the signal handler is now called from the main loop you don't need to use this message anymore and can do what happens during the handling of the message here already.
(In reply to comment #6) > Review of attachment 235842 [details] [review]: > > ::: tools/gst-launch.c > @@ +38,3 @@ > #endif > #ifndef DISABLE_FAULT_HANDLER > +#include <glib-unix.h> > > Will this block only be used on Unix-like platforms? Yes: /* FIXME: hack alert */ #ifdef HAVE_WIN32 #define DISABLE_FAULT_HANDLER #endif Do you think it'd be a good idea to tidy this up (and SA_SIGINFO) by using autotools? Feels like both of them can be checked in configure.ac. > > @@ +517,3 @@ > + gst_message_new_application (GST_OBJECT (pipeline), > + gst_structure_new ("GstLaunchInterrupt", > + "message", G_TYPE_STRING, "Pipeline interrupted", NULL))); > > As the signal handler is now called from the main loop you don't need to use > this message anymore and can do what happens during the handling of the message > here already. I can't see any other way of exiting event_loop nicely other than posting a message. event_loop is entered (tried) several times. Any suggestions?
> Do you think it'd be a good idea to tidy this up (and SA_SIGINFO) by using > autotools? Feels like both of them can be checked in configure.ac. Sorry, what exactly do you want to check in configure.ac. From a maintenance perspective I'd like to get rid of as much code as possible, esp. platform-specific code, and avoid configure checks wherever possible. The system headers are already checked for though (HAVE_FOO_BAR_H in config.h). glib-unix.h should be used in case of #ifdef G_OS_UNIX (which should hopefully also be set on cygwin). siginfo/action stuff should go away if we use the GLib API, no? Don't know if that answers your question :)
(In reply to comment #8) > > Do you think it'd be a good idea to tidy this up (and SA_SIGINFO) by using > > autotools? Feels like both of them can be checked in configure.ac. > > Sorry, what exactly do you want to check in configure.ac. > Whether SA_SIGINFO flag is supported on a given platform. I can check in repo history what the original concerns were but I wanted to know if you have any strong feelings about it. > From a maintenance perspective I'd like to get rid of as much code as possible, > esp. platform-specific code, and avoid configure checks wherever possible. I'm the right guy to delete code. I love deleting code (not sure if more than writing it ;)) Sometimes I happen to delete too much so I solicit about code deletion in the first place. Good to know you'd like to get rid of as much code as you can :) > The > system headers are already checked for though (HAVE_FOO_BAR_H in config.h). The thing is that I was wondering if some configure tests are missing but I guess the right thing to do is simply to get rid of SA_SIGINFO altogether as there's not much use of it other than printing some diagnostic info. > glib-unix.h should be used in case of #ifdef G_OS_UNIX (which should hopefully > also be set on cygwin). Oh, great. I didn't know that (as I'm not a glib guru... and not guru at all :)). > siginfo/action stuff should go away if we use the GLib > API, no? I guess for SIGSEGV and SIGQUIT the old school signal handling code has to stay to keep the nice feature that enables attaching a debugger to a spinning process that is about to die. GLib API doesn't support these signals. Unless you think otherwise. > Don't know if that answers your question :) Hm... sort of. I'll try to do the right thing and we'll see how it goes with a next patch.
Created attachment 235967 [details] [review] A new patch that rips out raw signal handling and uses g_source_set_callback() instead OK, another round. This time though all the FIXME chaff removed: - using G_OS_UNIX standard macro - removed SA_SIGINFO use as it was of a little value - kept traditional signal handling for SIGSEGV and SIGQUIT
commit 6099b35f7e64a80050d23fb224b1fd6b22a5f88d Author: Krzysztof Konopko <krzysztof.konopko@gmail.com> Date: Wed Feb 13 00:27:28 2013 +0000 gst-launch: Use g_unix_signal_add() to handle keyboard interruption Current implementation uses a traditional signal handler and a 250ms timeout callback in the event loop. Adding a GSource with g_unix_signal_add() to the GMainLoop is a much more elegant solution. The signal handler with this approach can send a message to the bus directly rather than set a flag as all dispatching intricacies are handled by GLib. https://bugzilla.gnome.org/show_bug.cgi?id=693481