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

[Uri Lublin]

On 07/06/2012 01:28 AM, Marc-André Lureau wrote:
> 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

In V4, I moved it (reference incremental) to 
spice_usb_device_manager_connect_device_async.

In V5, I keep it here, where it's correct.

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

Since I expect this operation to not occur many times, I used g_message.
I removed that code in V5.

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

ok. In V5 the installer does not register for cancellation events.
It just pass the cancellable to ginputstream and hopefully that's enough.


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

I did not fix it in V5.
Also I did not experience it.
Something for the future.

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

I'm not sure {0, } is less error-prone.
For example for static variables it can be confusing.
In V5, I modified it anyway.

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

Yes. It's the first write on this named-pipe, and a small one.
I expect it to not block.
If that's a problem, I'll change it to an async write.

>> +    /* 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?
ok. I added a g_return_if_fail on priv->result.
Every installer opens its own pipe and sends a single request for 
installing a driver.

>
>> +    priv->device = device;
> Wouldn't it make sense to ref it since the async operation may
> out-live the caller?

device is being ref'ed in usb-device-manager.

In V4, I used the same ref counted for the usb-connection, for the 
driver installation.
In V5, I separated them, such that the installer has its own ref/unref.
(even though it's equivalent, it seems to me people prefer it separated)


Thanks,
     Uri.