[Spice-devel] [spice-gtk Win32 v2 PATCH 4/5] Windows mingw: usb: Dynamically install a libusb driver for USB devices
Hans de Goede
hdegoede at redhat.com
Tue May 22 01:24:04 PDT 2012
Hi,
Comments inline.
On 05/20/2012 06:34 PM, Uri Lublin wrote:
> - Added win-usb-driver-install.[ch]
> - Added usbclerk.h
>
> Operation (on Windows, spice-gtk point of view):
> - After some sanity checks, just before redir'ing a USB device
> a libusb driver needs to be installed (before libusb can open/access it).
> - A connection (NamedPipe) is established with usb-clerk, a libusb
> driver installation service, and a request for driver installation
> is sent.
> - Installation status is asynchronously read from the pipe, and
> spice_usb_drv_install_finished() is called.
> - Upon a successful intallation USB devices are being rescanned
> by libusb (to pick up the new driver - required a libusbx patch),
> and the device usbredir continues.
>
> Linux operation is not changed.
> ---
> gtk/Makefile.am | 2 +
> gtk/usb-device-manager.c | 33 ++++++-
> gtk/usbclerk.h | 35 +++++++
> gtk/win-usb-driver-install.c | 213 ++++++++++++++++++++++++++++++++++++++++++
> gtk/win-usb-driver-install.h | 39 ++++++++
> 5 files changed, 321 insertions(+), 1 deletions(-)
> create mode 100644 gtk/usbclerk.h
> create mode 100644 gtk/win-usb-driver-install.c
> create mode 100644 gtk/win-usb-driver-install.h
>
> diff --git a/gtk/Makefile.am b/gtk/Makefile.am
> index 81150ab..67ab9d7 100644
> --- a/gtk/Makefile.am
> +++ b/gtk/Makefile.am
> @@ -313,6 +313,8 @@ WIN_USB_FILES= \
> win-usb-dev.h \
> win-usb-dev.c \
> usbclerk.h \
> + win-usb-driver-install.h \
> + win-usb-driver-install.c \
> $(NULL)
>
> if OS_WIN32
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index 2b6ce28..c373447 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -32,6 +32,7 @@
> #include<gudev/gudev.h>
> #elif defined(G_OS_WIN32)
> #include "win-usb-dev.h"
> +#include "win-usb-driver-install.h"
> #else
> #warning "Expecting one of G_OS_WIN32 and USE_GUDEV to be defined"
> #endif
> @@ -593,11 +594,16 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> priv->auto_conn_filter_rules_count,
> device, 0) == 0;
>
> - if (can_redirect&& auto_ok)
> + if (can_redirect&& auto_ok) {
> +#ifdef G_OS_WIN32
> + spice_win_usb_driver_install(self, device);
You probably want to ref device here, to ensure that it sticks around
until spice_usb_drv_install_finished gets called...
Also IMHO it would be a lot cleaner to give spice_win_usb_driver_install
a callback function to call, rather then having it hardcoded to
call spice_usb_drv_install_finished.
> +#else
> spice_usb_device_manager_connect_device_async(self,
> (SpiceUsbDevice *)device, NULL,
> spice_usb_device_manager_auto_connect_cb,
> libusb_ref_device(device));
> +#endif
> + }
> }
>
> SPICE_DEBUG("device added %p", device);
> @@ -661,6 +667,31 @@ static void spice_usb_device_manager_channel_connect_cb(
> g_object_unref(result);
> }
>
> +#ifdef G_OS_WIN32
> +/**
> + * spice_usb_drv_install_finished:
> + * @self: #SpiceUsbDeviceManager
> + * @device: the libusb device for which a libusb driver got installed
> + * @status: status of driver-installation operation
> + *
> + * Called when an Windows libusb driver installation completed.
> + *
> + * If the driver installation was successful, continue with USB
> + * device redirection
> + */
> +void spice_usb_drv_install_finished(SpiceUsbDeviceManager *self,
> + libusb_device *device, int status)
> +{
> + if (status) {
> + spice_win_usb_rescan_devices(self, self->priv->context);
> + spice_usb_device_manager_connect_device_async(self,
> + (SpiceUsbDevice *)device, NULL,
> + spice_usb_device_manager_auto_connect_cb,
> + libusb_ref_device(device));
You're taking the reference here, but at this point the device has
possibly already been destroyed.
I also think what we're seeing here explains why the whole rescan is not
working properly and you need the libusb patch for that. It would probably
be better to forget about the device completely, so remove it from the
usb-device-manager's list of devices and only remembering its bus and port nr,
and then re-add it after the driver install. Because now you're passing
device-info to spice_usb_device_manager_connect_device based on the device
state from before the driver installation.
###
I also note that you only handle auto-connect here, you should of course
also do a driver install for a manual connect, specifically it would be best
to move the whole driver install to spice_usb_device_manager_connect_device_async
so that it will work for both the manual and auto-connect codepaths and use
the same code for that.
> + }
> +}
> +#endif
> +
> /* ------------------------------------------------------------------ */
> /* private api */
>
> diff --git a/gtk/usbclerk.h b/gtk/usbclerk.h
> new file mode 100644
> index 0000000..5b1e3cf
> --- /dev/null
> +++ b/gtk/usbclerk.h
> @@ -0,0 +1,35 @@
> +#ifndef _H_USBCLERK
> +#define _H_USBCLERK
> +
> +#include<windows.h>
> +
> +#define USB_CLERK_PIPE_NAME TEXT("\\\\.\\pipe\\usbclerkpipe")
> +#define USB_CLERK_MAGIC 0xDADA
> +#define USB_CLERK_VERSION 0x0002
> +
> +typedef struct USBClerkHeader {
> + UINT16 magic;
> + UINT16 version;
> + UINT16 type;
> + UINT16 size;
> +} USBClerkHeader;
> +
> +enum {
> + USB_CLERK_DRIVER_INSTALL = 1,
> + USB_CLERK_DRIVER_REMOVE,
> + USB_CLERK_REPLY,
> + USB_CLERK_END_MESSAGE,
> +};
> +
> +typedef struct USBClerkDriverOp {
> + USBClerkHeader hdr;
> + UINT16 vid;
> + UINT16 pid;
> +} USBClerkDriverOp;
> +
> +typedef struct USBClerkReply {
> + USBClerkHeader hdr;
> + UINT32 status;
> +} USBClerkReply;
> +
> +#endif
> diff --git a/gtk/win-usb-driver-install.c b/gtk/win-usb-driver-install.c
> new file mode 100644
> index 0000000..07402c7
> --- /dev/null
> +++ b/gtk/win-usb-driver-install.c
> @@ -0,0 +1,213 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> + Copyright (C) 2011 Red Hat, Inc.
> +
> + Red Hat Authors:
> + Uri Lublin<uril at redhat.com>
> +
> + This library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + This library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with this library; if not, see<http://www.gnu.org/licenses/>.
> +*/
> +
> +#ifdef HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include<libusb.h>
> +#include "spice-util.h"
> +#include "usb-device-manager.h"
> +#include "usbclerk.h"
> +
> +typedef struct InstallInfo {
> + OVERLAPPED overlapped;
> + SpiceUsbDeviceManager *manager;
> + libusb_device *device;
> + HANDLE pipe;
> + USBClerkReply ack;
> +} InstallInfo;
> +
> +
> +/*
> + * The callback function to call when usb driver installation
> + * completes ( == upon a reply from usbclerk)
> + */
> +extern void spice_usb_drv_install_finished(
> + SpiceUsbDeviceManager *manager,
> + libusb_device *device,
> + int ack);
> +
> +
> +/**
> + * Get vid and pid of @device by reading its device descriptor
> + *
> + * Returns: TRUE upon success FALSE upon failure
> + */
> +static gboolean get_vid_pid(libusb_device *device, guint16 *pvid, guint16 *ppid)
> +{
> + struct libusb_device_descriptor devdesc;
> + gint r;
> +
> + g_return_val_if_fail(device != NULL&& pvid != NULL&& ppid != NULL, FALSE);
> + memset(&devdesc, 0, sizeof(devdesc));
> + r = libusb_get_device_descriptor(device,&devdesc);
> + if (r< 0) {
> + g_warning("libusb_get_device_descriptor failed");
> + return FALSE;
> + }
> + *pvid = devdesc.idVendor;
> + *ppid = devdesc.idProduct;
> + return TRUE;
> +}
> +
> +/**
> + *
> + * Handle a reply message from usbclerk, a service for driver installtion.
> + * Calls spice_usb_drv_install_finished.
> + */
> +void WINAPI handle_usb_service_reply(DWORD err, DWORD bytes,
> + LPOVERLAPPED ovlp)
> +{
> + InstallInfo *info = (InstallInfo*)ovlp;
> + SpiceUsbDeviceManager *manager = info->manager;
> + libusb_device *device = info->device;
> + int ack = info->ack.status;
> + int expected_bytes = sizeof(info->ack);
> +
> + /* first close the pipe */
> + SPICE_DEBUG("Finished reading: closing the named-pipe");
> + CloseHandle(info->pipe);
> +
> + if (info->ack.hdr.magic != USB_CLERK_MAGIC) {
> + g_warning("usbclerk magic mismatch: mine 0x%04x while server 0x%04x",
> + USB_CLERK_MAGIC, info->ack.hdr.magic);
> + ack = 0; /* fail */
> + }
> +
> + if (info->ack.hdr.version != USB_CLERK_VERSION) {
> + SPICE_DEBUG("version mismatch: mine 0x%04x while server 0x%04x",
> + USB_CLERK_VERSION, info->ack.hdr.version);
> + /* For now just warn, do not fail */
> + }
> +
> + if (info->ack.hdr.type != USB_CLERK_REPLY) {
> + g_warning("usbclerk message with unexpected type %d",
> + info->ack.hdr.type);
> + ack = 0; /* fail */
> + }
> +
> + SPICE_DEBUG("Finished reading: err=%ld bytes=%ld ack=%d",
> + err, bytes, ack);
> +
> + if (err || (bytes != expected_bytes) || !ack) {
> + g_warning("failed to read a message from pipe: "
> + "err=%ld bytes=%ld (expected=%d) ack=%d err=%ld",
> + err, bytes, expected_bytes, ack, GetLastError());
> + ack = 0; /* fail */
> + }
> +
> + SPICE_DEBUG("Calling usb-device-manager to continue with usb redir");
> + spice_usb_drv_install_finished(manager, device, ack);
> +
> + free(info);
> +}
> +
> +/**
> + *
> + * Start libusb driver installation for @device
> + *
> + * A new NamedPipe is created for each request.
> + *
> + * Returns: TRUE if a request was sent to usbclerk
> + * FALSE upon failure to send a request.
> + */
> +gboolean spice_win_usb_driver_install(SpiceUsbDeviceManager *manager,
> + libusb_device *device)
> +{
> + DWORD bytes;
> + HANDLE pipe;
> + USBClerkDriverOp req;
> + guint16 vid, pid;
> + InstallInfo *info = NULL;
> +
> + g_return_val_if_fail(manager != NULL&& device != NULL, FALSE);
> +
> + if (! get_vid_pid(device,&vid,&pid)) {
> + return FALSE;
> + }
> +
> + pipe = CreateFile(USB_CLERK_PIPE_NAME, GENERIC_READ | GENERIC_WRITE,
> + 0, NULL, OPEN_EXISTING, FILE_FLAG_OVERLAPPED, NULL);
> + if (pipe == INVALID_HANDLE_VALUE) {
> + g_warning("Cannot open pipe %s %ld", USB_CLERK_PIPE_NAME, GetLastError());
> + return FALSE;
> + }
> +
> + SPICE_DEBUG("Sending request to install libusb driver for %04x:%04x",
> + vid, pid);
> +
> + info = malloc(sizeof(*info));
> + if (!info) {
> + g_warning("FAILED to allocate memory for requesting driver installation");
> + goto install_failed;
> + }
> +
> + req.hdr.magic = USB_CLERK_MAGIC;
> + req.hdr.version = USB_CLERK_VERSION;
> + req.hdr.type = USB_CLERK_DRIVER_INSTALL;
> + req.hdr.size = sizeof(req);
> + req.vid = vid;
> + req.pid = pid;
It would be better to do the install based on bus and port numbers, what if the
user has 2 identical usb devices, and wants to redirect one?
> +
> + /* send request to the service */
> + if (!WriteFile(pipe,&req, sizeof(req),&bytes, NULL)) {
> + g_warning("Failed to write request to pipe err=%lu", GetLastError());
> + goto install_failed;
> + }
> + SPICE_DEBUG("Sent request of size %ld bytes to the service", bytes);
> +
> +
> + memset(info, 0, sizeof(*info));
> + info->manager = manager;
> + info->device = device;
> + info->pipe = pipe;
> + if (!ReadFileEx(pipe,&info->ack, sizeof(info->ack),&info->overlapped,
> + handle_usb_service_reply)) {
> + g_warning("Failed to read reply from pipe err=%lu", GetLastError());
> + goto install_failed;
> + }
> +
> + SPICE_DEBUG("Waiting (async) for a reply from usb service");
> +
> + return TRUE;
> +
> + install_failed:
> + CloseHandle(pipe);
> + free(info);
> +
> + return FALSE;
> +}
> +
> +gboolean spice_win_usb_rescan_devices(SpiceUsbDeviceManager *manager,
> + libusb_context *ctx)
> +{
> + libusb_device **dev_list = NULL;
> +
> + g_return_val_if_fail(manager != NULL&& ctx != NULL, FALSE);
> +
> + SPICE_DEBUG("rescanning libusb devices");
> + libusb_get_device_list(ctx,&dev_list);
> + if (dev_list)
> + libusb_free_device_list(dev_list, 1);
> + return TRUE;
> +}
> +
Maybe a strange question, but why not use glib functions for this ? These
functions will neatly follow the gio model including cancellation, etc.
Note that cancelling the read from the pipe will of course not actually
cancel the driver install being performed...
Regards,
Hans
More information about the Spice-devel
mailing list