<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 9 Feb 2016, at 23:43 PM, Jonathon Jongsma <<a href="mailto:jjongsma@redhat.com" class="">jjongsma@redhat.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">On Thu, 2015-10-29 at 17:27 +0200, Dmitry Fleytman wrote:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">From: Kirill Moizik <<a href="mailto:kmoizik@redhat.com" class="">kmoizik@redhat.com</a>><br class=""><br class="">On Windows when using usbdk, opening and closing USB device handle,<br class="">i.e. calling libusb_open()/libusb_unref_device() can block for a few<br class="">seconds (3-5 second more specifically on patch author's HW).<br class=""><br class="">libusb_open() is called by spice_usbredir_channel_open_device().<br class=""><br class="">libusb_unref_device() is called implicitely via<br class="">usbredirhost_set_device(..., NULL) from<br class="">spice_usbredir_channel_disconnect_device().<br class=""><br class="">Both these calls happen on the main thread. If it blocks,<br class="">this causes the UI to freeze.<br class=""><br class="">This commit makes sure libusb_open() is called from a different<br class="">thread to avoid blocking the mainloop freezes when usbdk is used.<br class=""><br class="">Following commits also move usbredirhost_set_device(..., NULL) call<br class="">to the different threads.<br class=""><br class="">Since this commit introduces additional execution contexts running in<br class="">parallell to the main thread threre are thread safety concerns to be secured.<br class="">Mainly there are 3 types of objects accessed by newly introduced threads:<br class=""> 1. libusb contexts<br class=""> 2. usbredir context<br class=""> 3. redirection channels<br class=""><br class="">Fortunately libusb accesses either thread safe of already performed by<br class="">a separate thread and protected by locks as needed.<br class=""><br class="">As for channels and usbredir, in order to achieve thread safety this patch<br class="">introduces<br class="">additional locks and references, namely:<br class=""><br class="">1. To make sure channel objects are not disposed while redirection in<br class="">progress,<br class=""> they are referenced on flow start and unreferenced on flow completion;<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I don't see this anywhere in this patch</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">2. Channel objects data accesses from different threads protected with a<br class=""> new lock (flows_mutex);<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">This refers to the earlier patches.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">3. Handling usbredir messages protected by the same new lock in order to<br class=""> ensure there are no messages processing flows in progres when device gets<br class=""> connected or disconneced.<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">is this also referring to a previous patch?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div>Indeed, this commit message is referring to an earlier series and became outdated.</div><div>Fixed in the next version.</div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class="">Signed-off-by: Kirill Moizik <<a href="mailto:kmoizik@redhat.com" class="">kmoizik@redhat.com</a>><br class="">Signed-off-by: Dmitry Fleytman <<a href="mailto:dfleytma@redhat.com" class="">dfleytma@redhat.com</a>><br class="">---<br class="">src/channel-usbredir.c | 41 ++++++++++++++++++++++++++++++-----------<br class="">1 file changed, 30 insertions(+), 11 deletions(-)<br class=""><br class="">diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c<br class="">index c88322a..302a2aa 100644<br class="">--- a/src/channel-usbredir.c<br class="">+++ b/src/channel-usbredir.c<br class="">@@ -315,6 +315,27 @@ static void spice_usbredir_channel_open_acl_cb(<br class="">}<br class="">#endif<br class=""><br class="">+#ifndef USE_POLKIT<br class="">+static void<br class="">+_open_device_async_cb(GSimpleAsyncResult *simple,<br class="">+ GObject *object,<br class="">+ GCancellable *cancellable)<br class="">+{<br class="">+ GError *err = NULL;<br class="">+ SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(object);<br class="">+ SpiceUsbredirChannelPrivate *priv = channel->priv;<br class="">+ g_mutex_lock(priv->flows_mutex);<br class="">+ if (!spice_usbredir_channel_open_device(channel, &err)) {<br class="">+ g_simple_async_result_take_error(simple, err);<br class="">+ libusb_unref_device(priv->device);<br class="">+ priv->device = NULL;<br class="">+ g_boxed_free(spice_usb_device_get_type(), priv->spice_device);<br class="">+ priv->spice_device = NULL;<br class="">+ }<br class="">+ g_mutex_unlock(priv->flows_mutex);<br class="">+}<br class="">+#endif<br class="">+<br class="">G_GNUC_INTERNAL<br class="">void spice_usbredir_channel_connect_device_async(<br class=""> SpiceUsbredirChannel *channel,<br class="">@@ -324,15 +345,14 @@ void spice_usbredir_channel_connect_device_async(<br class=""> GAsyncReadyCallback callback,<br class=""> gpointer user_data)<br class="">{<br class="">- SpiceUsbredirChannelPrivate *priv = channel->priv;<br class="">+ SpiceUsbredirChannelPrivate *priv;<br class=""> GSimpleAsyncResult *result;<br class="">-#if ! USE_POLKIT<br class="">- GError *err = NULL;<br class="">-#endif<br class=""><br class=""> g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));<br class=""> g_return_if_fail(device != NULL);<br class=""><br class="">+ priv = channel->priv;<br class="">+<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I would just drop this hunk.<span class="Apple-converted-space"> </span></span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div><br class=""></div><div>Dropped, thanks.</div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""> CHANNEL_DEBUG(channel, "connecting device %04x:%04x (%p) to channel %p",<br class=""> spice_usb_device_get_vid(spice_device),<br class=""> spice_usb_device_get_pid(spice_device),<br class="">@@ -372,13 +392,12 @@ void spice_usbredir_channel_connect_device_async(<br class=""> channel);<br class=""> return;<br class="">#else<br class="">- if (!spice_usbredir_channel_open_device(channel, &err)) {<br class="">- g_simple_async_result_take_error(result, err);<br class="">- libusb_unref_device(priv->device);<br class="">- priv->device = NULL;<br class="">- g_boxed_free(spice_usb_device_get_type(), priv->spice_device);<br class="">- priv->spice_device = NULL;<br class="">- }<br class="">+ g_simple_async_result_run_in_thread(result,<br class="">+ _open_device_async_cb,<br class="">+ G_PRIORITY_DEFAULT,<br class="">+ cancellable);<br class="">+ g_object_unref(result);<br class="">+ return;<br class="">#endif<br class=""><br class="">done:<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">The code looks fine, but I think the commit log needs to be revised a little bit</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">due to (presumably) patches being split</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div><br class=""></div><div>Done.</div><br class=""><blockquote type="cite" class=""><div class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Reviewed-by: Jonathon Jongsma <</span><a href="mailto:jjongsma@redhat.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">jjongsma@redhat.com</a><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">></span></div></blockquote></div><br class=""></body></html>