[Spice-devel] [spice-gtk v1 1/2] Revert "channel-usbredir: Fix crash on channel-up"

[Victor Toso]

Hi,

On Wed, Apr 25, 2018 at 04:05:00AM -0400, Frediano Ziglio wrote:
> > 
> > Ping
> > 
> 
> Looking back at the comments looks like this new set is still
> not doing the right thing and you agree, which is confusing to
> me.

Well, spice-session holds the reference to SpiceUsbDeviceManager
and has API to give that reference to different
SpiceUsbredirChannel so I do think it should be fine to
initialize it which solves that rhbz#1399838 bug.

But while doing that, regression on migration, rhbz#1558043,
which is not easy to solve while keeping that patch.

> It's replacing a crash with a critical warning ignoring the
> problem. Well... maybe this is actually a progress.

Indeed, I would rather not have the regression while replacing a
crash with a critical, yes.

> Maybe if you have an idea how it should behave at the end after
> removing the critical opening a new bug and stating in the new
> commit would be more clear.
> 
> Would be good also to have an idea of the different cases we
> should handle.

Besides avoiding regression... SpiceUsbDeviceManager is only
initialized on UI objects in spice-gtk. That should be fixed at
some point as non gtk tools like spicy-stats might do usb
redirection.

If this patches seems like good progress, I'll file a bug to fix
the critical that this patch set introduces, yes :)

Cheers,
        toso

> > On Fri, Apr 06, 2018 at 09:59:43AM +0200, Victor Toso wrote:
> > > From: Victor Toso <me at victortoso.com>
> > > 
> > > This reverts commit 291f3e4419e6fb4077ae43a5e09eb1c37b9dd729
> > > 
> > > Follow up patch should address rhbz#1399838 mentioned in commit above
> > > in a different way.
> > > 
> > > Major reason to revert is that the SpiceUsbDeviceManager object is
> > > kept in SpiceSession as an easy way to share it between different
> > > SpiceUsbredirChannel while SpiceSession itself does not use it. This
> > > causes problems on migration as we start a new session with a new
> > > SpiceUsbDeviceManager object while the previous one still exists.
> > > 
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1558043
> > > ---
> > >  src/spice-session.c | 7 -------
> > >  1 file changed, 7 deletions(-)
> > > 
> > > diff --git a/src/spice-session.c b/src/spice-session.c
> > > index 57acc63..f856244 100644
> > > --- a/src/spice-session.c
> > > +++ b/src/spice-session.c
> > > @@ -280,7 +280,6 @@ static void spice_session_init(SpiceSession *session)
> > >  {
> > >      SpiceSessionPrivate *s;
> > >      gchar *channels;
> > > -    GError *err = NULL;
> > >  
> > >      SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
> > >      s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
> > > @@ -293,12 +292,6 @@ static void spice_session_init(SpiceSession *session)
> > >      s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> > >      s->glz_window = glz_decoder_window_new();
> > >      update_proxy(session, NULL);
> > > -
> > > -    s->usb_manager = spice_usb_device_manager_get(session, &err);
> > > -    if (err != NULL) {
> > > -        SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s",
> > > err->message);
> > > -        g_clear_error(&err);
> > > -    }
> > >  }
> > >  
> > >  static void
> 
> Frediano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180425/6ce5b244/attachment.sig>