<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Jul 7, 2015 at 3:51 PM, Christophe Fergeau <<a href="mailto:cfergeau@redhat.com" target="_blank">cfergeau@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Jul 06, 2015 at 08:59:06PM +0300, Kirill Moizik wrote: > we will spawn a separate thread to disconnect usb device, when finish we will update widget to allow further > usb redirection operations > 1) expose spice_usbredir_channel_connect_device_async function for asynchronous disconnect > 2) threads synchronization Honestly, moving non-trivial existing functions from running in the main thread to running in a separate thread with very little locking added, and no high level explanation as to why everything will be fine and nothing can be racy is very scary... One example below: </blockquote><div> </div><div>I understand, I new to this project so i can't give you high level explanation why there is no races. I am aware there could be races, so i spent time trying to find it. I tried few Os's on different hardware, with and without spice/gtk debugging flags to change timing of flows to see if it will fail, but l it worked for me. Also i was trying to add another locks in suspicious places, but it led to deadlocks. Do you have any ideas how to prove quality of those patches ?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > > Signed-off-by: Kirill Moizik <<a href="mailto:kmoizik@redhat.com">kmoizik@redhat.com</a>> > --- > src/channel-usbredir-priv.h | 8 ++++- > src/channel-usbredir.c | 79 ++++++++++++++++++++++++++++++++++++++------- > src/map-file | 1 + > src/usb-device-manager.c | 62 ++++++++++++++++++++++++++++++++++- > src/usb-device-manager.h | 8 +++++ > src/usb-device-widget.c | 19 +++++++++-- > 6 files changed, 161 insertions(+), 16 deletions(-) > > diff --git a/src/channel-usbredir-priv.h b/src/channel-usbredir-priv.h > index 2c4c6f7..c3ff158 100644 > --- a/src/channel-usbredir-priv.h > +++ b/src/channel-usbredir-priv.h > @@ -33,6 +33,10 @@ G_BEGIN_DECLS > void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel, > libusb_context *context); > > +void spice_usbredir_channel_disconnect_device_async(SpiceUsbredirChannel *channel, > + GSimpleAsyncResult *simple, > + GCancellable *cancellable); > + > /* Note the context must be set, and the channel must be brought up > (through spice_channel_connect()), before calling this. */ > void spice_usbredir_channel_connect_device_async( > @@ -47,7 +51,9 @@ gboolean spice_usbredir_channel_connect_device_finish( > GAsyncResult *res, > GError **err); > > -void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel); > +void spice_usbredir_channel_disconnect_device(GSimpleAsyncResult *simple, > + GObject *object, > + GCancellable *cancellable); > > libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel); > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c > index e7d5949..1e78350 100644 > --- a/src/channel-usbredir.c > +++ b/src/channel-usbredir.c > @@ -79,6 +79,7 @@ struct _SpiceUsbredirChannelPrivate { > GSimpleAsyncResult *result; > SpiceUsbAclHelper *acl_helper; > #endif > + void* redirect_mutex; </div></div>usbredir_alloc_lock returns a void* only because that's what usbredir expects. Let's just cast it to GMutex * here, and use g_mutex_lock/unlock. </blockquote><div> </div><div>Ok, will fix it </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> [snip] > @@ -440,7 +491,9 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel) > spice_usb_device_manager_get(session, NULL)); > } > /* This also closes the libusb handle we passed from open_device */ > + usbredir_lock_lock(channel->priv->redirect_mutex); > usbredirhost_set_device(priv->host, NULL); > + usbredir_unlock_lock(channel->priv->redirect_mutex); > libusb_unref_device(priv->device); > priv->device = NULL; > g_boxed_free(spice_usb_device_get_type(), priv->spice_device); > priv->spice_device = NULL; > > @@ -652,7 +705,9 @@ static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in) > priv->read_buf = buf; > priv->read_buf_size = size; > > + usbredir_lock_lock(priv->redirect_mutex); > r = usbredirhost_read_guest_data(priv->host); > + usbredir_unlock_lock(priv->redirect_mutex); > if (r != 0) { > SpiceUsbDevice *spice_device = priv->spice_device; > gchar *desc; > GError *err; > g_return_if_fail(spice_device != NULL); The 2 hunks I kept are the only 2 places loking/unlocking redirect_mutex, I assume this means they can race with each other. If that's true, then we can get in a situation where 2nd hunk code runs till if (r != 0) { with r != 0, then 1st hunk runs and sets priv->spice_device to NULL, then 2nd hunk resumes, and we trigger the g_return_if_fail() (which usually is used to indicate a programming error) </blockquote><div> </div><div>we can expand critical section in this case, is it bad idea? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Christophe </blockquote></div> </div></div>