[Spice-devel] [spice-gtk Win32 v4 06/17] Windows mingw: usb: Dynamically install a libusb driver for USB devices

[Marc-André Lureau]

Hi

some remarks below

On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin <uril at redhat.com> wrote:
> @@ -635,7 +649,7 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>              spice_usb_device_manager_connect_device_async(self,
>                                     device, NULL,
>                                     spice_usb_device_manager_auto_connect_cb,
> -                                   g_object_ref(device));
> +                                   device);

This is a bit worrisome, an async operation should keep a reference on
device, or it must make sure to cancel cleanly when the device is
destroyed.

The spice_usb_device_manager_auto_connect_cb is still doing
spice_usb_device_unref(device), is that normal? see also comment in
03/17

> +    installer = spice_win_usb_driver_new();
> +    cbinfo = g_new0(UsbInstallCbInfo, 1);
> +    cbinfo->manager     = self;
> +    cbinfo->device      = device;
> +    cbinfo->installer   = installer;
> +    cbinfo->cancellable = cancellable;
> +    cbinfo->callback    = callback;
> +    cbinfo->user_data   = user_data;
> +    spice_win_usb_driver_install(installer, device, NULL,
> +                                 spice_usb_device_manager_drv_install_cb,
> +                                 cbinfo);

The same cancellable should be used.


> +static void win_usb_driver_cancelled_cb(GCancellable *cancellable, gpointer user_data)
> +{
> +    SpiceWinUsbDriver *self = SPICE_WIN_USB_DRIVER(user_data);
> +    SpiceWinUsbDriverPrivate *priv = self->priv;
> +
> +    g_message("IN %s result=%p", __FUNCTION__, priv->result);

s/g_message/SPICE_DEBUG

> +    if (priv->result) {
> +        win_usb_driver_async_result_set_cancelled(priv->result);
> +        g_simple_async_result_complete_in_idle(priv->result);
> +    }

I think this isn't really cancelling the operation, it will give the
impression to the caller that it is cancelled, by returning an error.
But there will still be an outstanding/ pending operation that may
cause further errors when calling the clerk etc, the device might be
busy or there might be multiple simulatenous requests etc...

> +    istream = (GInputStream *)gobject;

prefer the safer G_INPUT_STREAM macro over a static cast.

> +
> +    if (bytes != sizeof(priv->reply)) {
> +        g_warning("usbclerk size mismatch: read %d bytes, expected %d (header %d, size in header %d)",
> +                  bytes, sizeof(priv->reply), sizeof(priv->reply.hdr), priv->reply.hdr.size);
> +        /* For now just warn, do not fail */

Although a bit unlikely, this read might be incomplete, so you may
want to re-call read_async with the rest of the buffer in this case
(it seems to happen quite often with the legacy usb)

> +    memset(&req, 0, sizeof(req));

Just a note because it was there a couple of time, a declaration such
as USBClerkDriverOp req = { 0, } is less error-prone and elegant than
calling memset() yourself. No problem!

> +    req.hdr.magic   = USB_CLERK_MAGIC;
> +    req.hdr.version = USB_CLERK_VERSION;
> +    req.hdr.type    = op;
> +    req.hdr.size    = sizeof(req);
> +    req.vid = vid;
> +    req.pid = pid;
> +
> +    ostream = g_win32_output_stream_new(priv->handle, FALSE);
> +
> +    ret = g_output_stream_write_all(ostream, &req, sizeof(req), &bytes, NULL, err);

This is a blocking call, in the main thread, afaict

> +    if (!spice_win_usb_driver_send_request(self, USB_CLERK_DRIVER_INSTALL,
> +                                           vid, pid, &err)) {

So this is blocking op

> +    /* set up for async read */
> +    priv->result = result;

So what happen if there is already an outstanding priv->result?
perhaps it should g_return_if_fail condition at the beginning of this
function?

> +    priv->device = device;

Wouldn't it make sense to ref it since the async operation may
out-live the caller?

> +    if (cancellable) {
> +        priv->cancellable = cancellable;
> +        priv->cancellable_id = g_cancellable_connect(cancellable,
> +                                                     G_CALLBACK(win_usb_driver_cancelled_cb),
> +                                                     self, NULL);
> +    }

This isn't really helping, the signal isn't going to cancel the
operation on a different thread, the cancellable should be passed to
all the async operations.

> +    spice_win_usb_driver_read_reply_async(self);

Here


-- 
Marc-André Lureau