After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 693481 - gst-launch: Use signalfd() to handle keyboard interruption
gst-launch: Use signalfd() to handle keyboard interruption
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-02-09 16:23 UTC by Krzysztof Konopko
Modified: 2013-02-14 09:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (6.63 KB, patch)
2013-02-09 16:23 UTC, Krzysztof Konopko
none Details | Review
A new patch that rips out raw signal handling and uses g_source_set_callback() instead (4.73 KB, patch)
2013-02-13 00:33 UTC, Krzysztof Konopko
needs-work Details | Review
A new patch that rips out raw signal handling and uses g_source_set_callback() instead (6.99 KB, patch)
2013-02-13 23:49 UTC, Krzysztof Konopko
committed Details | Review

Description Krzysztof Konopko 2013-02-09 16:23:56 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?
Comment 1 Krzysztof Konopko 2013-02-09 16:42:31 UTC
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.
Comment 2 Tim-Philipp Müller 2013-02-09 18:04:28 UTC
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.
Comment 3 Krzysztof Konopko 2013-02-09 21:22:56 UTC
Tempting.  I'll have a look.
Comment 4 Krzysztof Konopko 2013-02-13 00:33:32 UTC
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
Comment 5 Krzysztof Konopko 2013-02-13 00:40:37 UTC
(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().
Comment 6 Sebastian Dröge (slomo) 2013-02-13 09:05:28 UTC
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.
Comment 7 Krzysztof Konopko 2013-02-13 15:17:29 UTC
(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?
Comment 8 Tim-Philipp Müller 2013-02-13 17:14:33 UTC
> 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 :)
Comment 9 Krzysztof Konopko 2013-02-13 20:00:30 UTC
(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.
Comment 10 Krzysztof Konopko 2013-02-13 23:49:39 UTC
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
Comment 11 Sebastian Dröge (slomo) 2013-02-14 09:12:01 UTC
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