[Spice-devel] [PATCH spice-gtk v2] session: initialize USB device manager on session creation

[Marc-André Lureau]

On Thu, Jan 3, 2019 at 8:12 PM Christophe Fergeau <cfergeau at redhat.com> wrote:
>
> Hey,
>
> On Thu, Jan 03, 2019 at 03:41:35PM +0400, marcandre.lureau at redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> >
> > Simple command line tools simply connect all channels, however
> > usbredir channels require that the manager was previously
> > initialized. Currently, running spicy-stats on a VM with usbredir
> > channels prints:
> >
> > (lt-spicy-stats:25224): GSpice-CRITICAL **: 14:30:54.724: spice_usbredir_channel_up: assertion 'priv->host != NULL' failed
> >
> > (lt-spicy-stats:25224): GSpice-CRITICAL **: 14:30:54.724: usbredir_handle_msg: assertion 'priv->host != NULL' failed
> >
> > There is not strong reason not to initialize the USB device manager
> > when the session is created.
> >
> > Notes:
> > - when usbredir isn't compiled in, those criticals aren't reached
> > - a previous attempt was to initialize the usb device manager during
> >   session instance init, however the manager shouldn't interact with a
> >   temporary migration session
>
> Looks good to me (it's very similar to 291f3e4 except that the code only
> runs in spice_session_new, and not in spice_session_new_from_session as
> was happening when this code was part of spice_session_init).
>
> I've tested migration + usbredir and spicy-stats, and I did not hit any
> of the 2 bugs we had in the past (and I've checked that without " Revert
> "channel-usbredir: Fix crash on channel-up" I can still reproduce the
> migration bug, and without your patch, I get the criticals from the
> commit log).
>
> Reviewed-by: Christophe Fergeau <cfergeau at redhat.com>

Thanks Christophe and Victor for the review!

>
> Christophe
>
>
> >
> > Fixes:
> > https://gitlab.freedesktop.org/spice/spice-gtk/issues/67
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > ---
> >  src/spice-session.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index a2ed401..76c90b3 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -1492,7 +1492,17 @@ static void spice_session_class_init(SpiceSessionClass *klass)
> >   **/
> >  SpiceSession *spice_session_new(void)
> >  {
> > -    return SPICE_SESSION(g_object_new(SPICE_TYPE_SESSION, NULL));
> > +    SpiceSession *self = SPICE_SESSION(g_object_new(SPICE_TYPE_SESSION, NULL));
> > +    SpiceSessionPrivate *priv = self->priv;
> > +    GError *err = NULL;
> > +
> > +    priv->usb_manager = spice_usb_device_manager_get(self, &err);
> > +    if (err != NULL) {
> > +        SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", err->message);
> > +        g_clear_error(&err);
> > +    }
> > +
> > +    return self;
> >  }
> >
> >  G_GNUC_INTERNAL
> > --
> > 2.20.1.2.gb21ebb671b
> >
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau