Hi
some remarks below
On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin <uril@xxxxxxxxxx> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel