Re: [PATCH spice-gtk 14/14] win-usb: try to connect to usbclerk during init

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Thu, Apr 24, 2014 at 4:37 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
Perhaps there should be a g_warning here to aid debugging?  Or alternately in the function that calls this one?


The spice_usb_device_manager_get() will return NULL , with a GError. Since this function is called many times by spice-gtk or the client, I think it's preferable to let the application print a warning or a dialog (once) instead of flooding with warnings. However, I added a SPICE_DEBUG.

>          return FALSE;
> -    }
>  #endif
>
>      /* Initialize libusb */
> diff --git a/gtk/win-usb-driver-install.c b/gtk/win-usb-driver-install.c
> index bb18ae4..71b51d7 100644
> --- a/gtk/win-usb-driver-install.c
> +++ b/gtk/win-usb-driver-install.c
> @@ -53,14 +53,42 @@ struct _SpiceWinUsbDriverPrivate {
>  };
>
>
> +static void spice_win_usb_driver_initable_iface_init(GInitableIface *iface);
>
> -G_DEFINE_TYPE(SpiceWinUsbDriver, spice_win_usb_driver, G_TYPE_OBJECT);
> +G_DEFINE_TYPE_WITH_CODE(SpiceWinUsbDriver, spice_win_usb_driver,
> G_TYPE_OBJECT,
> +    G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE,
> spice_win_usb_driver_initable_iface_init));
>
>  static void spice_win_usb_driver_init(SpiceWinUsbDriver *self)
>  {
>      self->priv = SPICE_WIN_USB_DRIVER_GET_PRIVATE(self);
>  }
>
> +static gboolean spice_win_usb_driver_initable_init(GInitable     *initable,
> +                                                   GCancellable
> *cancellable,
> +                                                   GError        **err)
> +{
> +    SpiceWinUsbDriver *self = SPICE_WIN_USB_DRIVER(initable);
> +    SpiceWinUsbDriverPrivate *priv = self->priv;
> +
> +    SPICE_DEBUG("win-usb-driver-install: connecting to usbclerk named
> pipe");
> +    priv->handle = CreateFile(USB_CLERK_PIPE_NAME,
> +                              GENERIC_READ | GENERIC_WRITE,
> +                              0, NULL,
> +                              OPEN_EXISTING,
> +                              FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OVERLAPPED,
> +                              NULL);
> +    if (priv->handle == INVALID_HANDLE_VALUE) {
> +        DWORD errval  = GetLastError();
> +        gchar *errstr = g_win32_error_message(errval);
> +        g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_USB_SERVICE,
> +                    "Failed to create service named pipe (%ld) %s", errval,
> errstr);
> +        g_free(errstr);
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
>  static void spice_win_usb_driver_finalize(GObject *gobject)
>  {
>      SpiceWinUsbDriver *self = SPICE_WIN_USB_DRIVER(gobject);
> @@ -68,6 +96,7 @@ static void spice_win_usb_driver_finalize(GObject *gobject)
>
>      if (priv->handle)
>          CloseHandle(priv->handle);
> +
>      g_clear_object(&priv->result);
>  }
>
> @@ -80,6 +109,11 @@ static void
> spice_win_usb_driver_class_init(SpiceWinUsbDriverClass *klass)
>      g_type_class_add_private(klass, sizeof(SpiceWinUsbDriverPrivate));
>  }
>
> +static void spice_win_usb_driver_initable_iface_init(GInitableIface *iface)
> +{
> +    iface->init = spice_win_usb_driver_initable_init;
> +}
> +
>  /* ------------------------------------------------------------------ */
>  /* callbacks                                                          */
>
> @@ -244,13 +278,15 @@ void
> spice_win_usb_driver_read_reply_async(SpiceWinUsbDriver *self)
>
>
>  G_GNUC_INTERNAL
> -SpiceWinUsbDriver *spice_win_usb_driver_new(void)
> +SpiceWinUsbDriver *spice_win_usb_driver_new(GError **err)
>  {
> -    GObject *obj;
> +    GObject *self;
> +
> +    g_return_val_if_fail(err == NULL || *err == NULL, FALSE);
>
> -    obj = g_object_new(SPICE_TYPE_WIN_USB_DRIVER, NULL);
> +    self = g_initable_new(SPICE_TYPE_WIN_USB_DRIVER, NULL, err, NULL);
>
> -    return SPICE_WIN_USB_DRIVER(obj);
> +    return SPICE_WIN_USB_DRIVER(self);
>  }
>
>  static
> @@ -286,26 +322,6 @@ void spice_win_usb_driver_op(SpiceWinUsbDriver *self,
>      vid = spice_usb_device_get_vid(device);
>      pid = spice_usb_device_get_pid(device);
>
> -    if (! priv->handle ) {
> -        SPICE_DEBUG("win-usb-driver-install: connecting to usbclerk named
> pipe");
> -        priv->handle = CreateFile(USB_CLERK_PIPE_NAME,
> -                                  GENERIC_READ | GENERIC_WRITE,
> -                                  0, NULL,
> -                                  OPEN_EXISTING,
> -                                  FILE_ATTRIBUTE_NORMAL |
> FILE_FLAG_OVERLAPPED,
> -                                  NULL);
> -        if (priv->handle == INVALID_HANDLE_VALUE) {
> -            DWORD errval  = GetLastError();
> -            gchar *errstr = g_win32_error_message(errval);
> -            g_warning("failed to create a named pipe to usbclerk (%ld) %s",
> -                      errval,errstr);
> -            g_simple_async_result_set_error(result,
> -                      G_IO_ERROR, G_IO_ERROR_FAILED,
> -                      "Failed to create named pipe (%ld) %s", errval,
> errstr);
> -            goto failed_request;
> -        }
> -    }
> -

One behavioral change to note here: with the old code, if we failed to create the usbclerk pipe, we would re-try to do it again the next time spice_win_usb_driver_op() was called.  With the new code, we try it once at the beginning and then never try again. I suspect that re-trying isn't really useful (if it failed the first time, it will likely fail again the next time), but I thought it was worth mentioning the change in behavior.

Yes, that's correct. I added a comment in the commit message (now it retries every time UsbDeviceManager is initialized).


--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]