[virt-viewer PATCH v2] remote-viewer: add handler for SIGINT signal

[Daniel P. Berrangé]

On Fri, Jan 17, 2020 at 12:04:08PM +0100, Francesco Giudici wrote:
> When remote-viewer is started from terminal, CTRL-C sends a SIGINT
> signal to the program causing immediate termination. On linux clients
> usb redirected devices are left without any kernel driver attached,
> causing them to appear as no more available to the host.
> Add a SIGINT handler to allow a clean exit, in particular to allow
> the kernel to attach back the host driver.
> The issue is present on linux only.
> 
> To perform usb device redirection, virt-viewer leverages spice-gtk
> library, which in turn relies on usbredir library, which uses libusb.
> In order to take control of the usb device the auto-loaded kernel
> driver must be detached. This is achieved (in the very end) with
> libusb_detach_kernel_driver(). Then the device interfaces can be claimed
> with libusb_claim_interface() and get in control to the application.
> During normal application termination, the usb channel is teared down,
> performing a reset of the usb device and giving back the control of the
> device to the kernel (libusb_attach_kernel_driver()).
> If the application quits without doing this, the device interfaces will
> end up with no driver attached, making them not usable in the host
> system.
> 
> Note that enabling libusb_set_auto_detach_kernel_driver() does not solve
> the issue, as this is just a convenient API from libusb: libusb will
> take care of detaching/attaching the driver to the interfaces of the usb
> device each time a call to libusb_release_interface() and
> libusb_claim_interface() is performed. An unexpected quit of the
> application will skip the libusb_release_interface() call too, leaving
> the interfaces without any driver attached.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1713311
> 
> Signed-off-by: Francesco Giudici <fgiudici at redhat.com>
> ---
>  src/virt-viewer-app.c | 79 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> index da8cfa9..06e237b 100644
> --- a/src/virt-viewer-app.c
> +++ b/src/virt-viewer-app.c
> @@ -36,6 +36,7 @@
>  #include <glib/gprintf.h>
>  #include <glib/gi18n.h>
>  #include <errno.h>
> +#include <fcntl.h>
>  
>  #ifdef HAVE_SYS_SOCKET_H
>  #include <sys/socket.h>
> @@ -1756,6 +1757,74 @@ static gboolean opt_fullscreen = FALSE;
>  static gboolean opt_kiosk = FALSE;
>  static gboolean opt_kiosk_quit = FALSE;
>  
> +#ifndef G_OS_WIN32
> +static int sigint_pipe[2];
> +
> +static void
> +sigint_handler(int signum)
> +{
> +    int savedErrno;
> +
> +    g_return_if_fail(signum == SIGINT);
> +
> +    savedErrno = errno;
> +    if (write(sigint_pipe[1], "x", 1) == -1 && errno != EAGAIN)
> +        g_debug("SIGINT handler failure\n");
> +    errno = savedErrno;
> +}
> +
> +static void
> +register_sigint_handler()
> +{
> +    int flags, i;
> +    struct sigaction sa;
> +
> +    if (pipe(sigint_pipe) == -1)
> +        goto err;
> +
> +    for (i = 0; i < 2; i++) {
> +        flags = fcntl(sigint_pipe[i], F_GETFL);
> +        if (flags == -1)
> +            goto err;
> +        flags |= O_NONBLOCK;
> +        if (fcntl(sigint_pipe[i], F_SETFL, flags) == -1)
> +            goto err;
> +    }
> +
> +    sigemptyset(&sa.sa_mask);
> +    sa.sa_flags = SA_RESTART;
> +    sa.sa_handler = sigint_handler;
> +    if (sigaction(SIGINT, &sa, NULL) == -1)
> +        goto err;
> +
> +    return;
> +
> +err:
> +    g_debug("Cannot register SIGINT handler\n");
> +}
> +
> +static gboolean
> +sigint_cb(GIOChannel *source,
> +          GIOCondition condition,
> +          gpointer data)
> +{
> +    VirtViewerApp *self = VIRT_VIEWER_APP(data);
> +    VirtViewerAppPrivate *priv = self->priv;
> +    gchar sbuf;
> +
> +    g_assert(condition == G_IO_IN);
> +
> +    g_debug("got SIGINT, quitting\n");
> +    if (priv->started)
> +        virt_viewer_app_quit(self);
> +    else
> +        exit(0);
> +
> +    g_io_channel_read_chars (source, &sbuf, 1, NULL, NULL);
> +    return TRUE;
> +}
> +#endif
> +
>  static void
>  title_maybe_changed(VirtViewerApp *self, GParamSpec* pspec G_GNUC_UNUSED, gpointer user_data G_GNUC_UNUSED)
>  {
> @@ -1766,10 +1835,20 @@ static void
>  virt_viewer_app_init(VirtViewerApp *self)
>  {
>      GError *error = NULL;
> +#ifndef G_OS_WIN32
> +    GIOChannel *sigint_channel = NULL;
> +#endif
> +
>      self->priv = virt_viewer_app_get_instance_private(self);
>  
>      gtk_window_set_default_icon_name("virt-viewer");
>  
> +#ifndef G_OS_WIN32
> +    register_sigint_handler();
> +    sigint_channel = g_io_channel_unix_new(sigint_pipe[0]);
> +    g_io_add_watch(sigint_channel, G_IO_IN, sigint_cb, self);
> +#endif

Just yesterday I learnt that GLib has native support for signal handling
in its event loop which allows this to be simplified:

   g_unix_signal_add (SIGINT, sigint_cb, self);

>      self->priv->displays = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_object_unref);
>      self->priv->config = g_key_file_new();
>      self->priv->config_file = g_build_filename(g_get_user_config_dir(),
> -- 
> 2.21.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|