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

[Francesco Giudici]

On 17/01/20 12:09, Daniel P. Berrangé wrote:
> 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);

Oh, right, this is much clean and easy.. initially I did the hard way to 
share the code with windows. But turned out we don't need Windows 
support here... pushing v3 patch in a while!

Thanks

Francesco

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