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@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
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.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel