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

[Frediano Ziglio]

> 
> 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 :)
> 

If you propose a solution with a "if the patches seems like good progress"
looks like you are not sure about it. You should propose good patches :-)

> 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.
> > > > 

Maybe SpiceUsbDeviceManager should be a global (singleton) meaning that you
should not create a new object but just add a reference to it even from
another SpiceSession?

> > > > 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
>