ACK with comments.
----- Original Message -----
> From: "Marc-André Lureau" <marcandre.lureau@xxxxxxxxx>
> To: spice-devel@xxxxxxxxxxxxxxx
> Sent: Wednesday, April 23, 2014 1:09:19 PM
> Subject: [PATCH spice-gtk 14/14] win-usb: try to connect to usbclerk during init
>
> Fail early during initialization if the usbclerk service can't be
> reached.
> ---
> gtk/usb-device-manager.c | 8 ++----
> gtk/win-usb-driver-install.c | 66
> +++++++++++++++++++++++++++-----------------
> gtk/win-usb-driver-install.h | 2 +-
> 3 files changed, 44 insertions(+), 32 deletions(-)
>
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index ec19564..fb2d102 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -253,13 +253,9 @@ static gboolean
> spice_usb_device_manager_initable_init(GInitable *initable,
> #ifdef USE_USBREDIR
>
> #ifdef G_OS_WIN32
> - priv->installer = spice_win_usb_driver_new();
> - if (!priv->installer) {
> - g_warn_if_reached();
> - g_set_error_literal(err, SPICE_CLIENT_ERROR,
> SPICE_CLIENT_ERROR_USB_SERVICE,
> - "Failed to initialize USB device installer
> service");
> + priv->installer = spice_win_usb_driver_new(err);
> + if (!priv->installer)
Perhaps there should be a g_warning here to aid debugging? Or alternately in the function that calls this one?
> 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.
> if (!spice_win_usb_driver_send_request(self, op_type,
> vid, pid, &err)) {
> g_warning("failed to send a request to usbclerk %s", err->message);
> diff --git a/gtk/win-usb-driver-install.h b/gtk/win-usb-driver-install.h
> index b9eadcd..f9afedc 100644
> --- a/gtk/win-usb-driver-install.h
> +++ b/gtk/win-usb-driver-install.h
> @@ -61,7 +61,7 @@ struct _SpiceWinUsbDriverClass
>
> GType spice_win_usb_driver_get_type(void);
>
> -SpiceWinUsbDriver *spice_win_usb_driver_new(void);
> +SpiceWinUsbDriver *spice_win_usb_driver_new(GError **err);
>
>
> void spice_win_usb_driver_install_async(SpiceWinUsbDriver *self,
> --
> 1.8.5.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel