Hi, On Wed, Nov 30, 2016 at 03:33:13PM +0100, Victor Toso wrote: > Hi, > > On Wed, Nov 30, 2016 at 11:20:42AM +0100, Christophe Fergeau wrote: > > On Tue, Nov 29, 2016 at 10:46:24PM +0100, Victor Toso wrote: > > > From: Victor Toso <me@xxxxxxxxxxxxxx> > > > > > > Seems that usbredirhost initialization can take longer then > > > the channel_up call from spice-channel. > > > > > > As this seems to be a race, a simple check for NULL pointer should be > > > enough. Similar fix was done in 5b252b0f499601bcf387c02a4dd35d27ed34c > > > > Too many "seems" for my taste in this commit log when this mentions a > > race condition :( > > Ah, yeah, sorry about that. I wrote it as I was thinking about other > possibilities too, leading the commit log to not be so certain about the > issue. > > > priv->host is initialized in spice_usbredir_channel_set_context() which > > is called from SpiceUsbDeviceManager::channel_new. SpiceUsbDeviceManager > > will only start listening for the "channel-new" signal in > > spice_usb_device_manager_initable_init(). > > > > I think what you are suggesting is that > > spice_usb_device_manager_initable_init() is taking too long, and the > > usbredir is up and running before we get a chance to set priv->host? > > Hm, it is a bit too hard to say for sure what is slow without more debug > info. > > As you said, what does initialize priv->host is: > 1-) channel-new [signal] -> channel_new() [usb-device-manager] > 2-) spice_usbredir_channel_set_context() > 3-) priv->host = usbredirhost_open_full(...) > > As this did not happen at the time of the crash, yes, maybe > spice_usb_device_manager_initable_init() failed or was not called > which would mean that spice_usb_device_manager_get() was not called. > > Not sure who should be calling spice_usb_device_manager_get() first? > - src/usb-device-widget.c at spice_usb_device_widget_constructor() > - src/spice-option.c .... at spice_set_session_option() > - src/spice-gtk-session.c at spice_gtk_session_request_auto_usbredir() > > But I'm sure it can't be on src/channel-usbredir.c at > spice_usbredir_channel_open_device() as this needs priv->host already :) > > > NB: channel_up is missing from the backtrace but is what is called > > between #0 and #1. > > Indeed, I wonder why is it missing? > > > Can we miss some data if we skip this flushing of pending writes? I > > guess we cannot have any since priv->host does not exist yet? > > I don't think we can miss anything if the device is not even open yet. > Maybe fixing this crash can trigger a different issue later on, > hopefully it would be easier to track. > > Let me know if I should improve the commit log but after writing this > email, the 'seems' are likely correct as I'm not 100% sure how this was > triggered. > > > > > Christophe > > Cheers, > toso I missed a information on bugzilla, see: Additional info: (...) cmdline: spicy-stats --uri=spice://localhost:5900 crash_function: usbredirhost_write_guest_data executable: /usr/bin/spicy-stats That should narrow down how the bug was triggered... > > > > > > > > > #0 in usbredirhost_write_guest_data (host=0x0) at usbredirhost.c:876 > > > #1 in spice_channel_recv_auth (channel=0x55af5f3b8f50 [SpiceUsbredirChannel]) at spice-channel.c:1225 > > > #2 in spice_channel_coroutine (data=0x55af5f3b8f50) at spice-channel.c:2580 > > > #3 in coroutine_trampoline (cc=0x55af5f3b85e0) at coroutine_ucontext.c:63 > > > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838 > > > > > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx> > > > --- > > > src/channel-usbredir.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c > > > index 4837d68..561f486 100644 > > > --- a/src/channel-usbredir.c > > > +++ b/src/channel-usbredir.c > > > @@ -817,6 +817,9 @@ static void spice_usbredir_channel_up(SpiceChannel *c) > > > SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c); > > > SpiceUsbredirChannelPrivate *priv = channel->priv; > > > > > > + if (priv->host == NULL) > > > + return; > > > + > > > /* Flush any pending writes */ > > > usbredirhost_write_guest_data(priv->host); > > > } > > > -- > > > 2.9.3 > > > > > > _______________________________________________ > > > Spice-devel mailing list > > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel