On Tue, Jul 7, 2015 at 3:51 PM, Christophe Fergeau <[email protected]> wrote:
> 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: > 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 ? > > > > > Signed-off-by: Kirill Moizik <[email protected]> > > --- > > 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; > > 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. > Ok, will fix it > > [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) > we can expand critical section in this case, is it bad idea? > > Christophe >
_______________________________________________ Spice-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/spice-devel
