[Spice-devel] [spice-gtk v1 2/4] usb-device-manager: Handle connectionless channel

[Victor Toso]

Hi,

On Fri, Sep 07, 2018 at 04:45:07AM -0400, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me at victortoso.com>
> > 
> > As we are not able to redirect anything in case that usbredir channel
> > is not connected.
> > 
> > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1625550
> > Signed-off-by: Victor Toso <victortoso at gnome.org>
> > ---
> >  src/usb-device-manager.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> > 
> > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > index 50fb491..8555af5 100644
> > --- a/src/usb-device-manager.c
> > +++ b/src/usb-device-manager.c
> > @@ -158,6 +158,8 @@ static void channel_new(SpiceSession *session,
> > SpiceChannel *channel,
> >                          gpointer user_data);
> >  static void channel_destroy(SpiceSession *session, SpiceChannel *channel,
> >                              gpointer user_data);
> > +static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> > +                          gpointer user_data);
> >  #ifdef USE_GUDEV
> >  static void spice_usb_device_manager_uevent_cb(GUdevClient     *client,
> >                                                 const gchar     *action,
> > @@ -849,6 +851,8 @@ static void channel_new(SpiceSession *session,
> > SpiceChannel *channel,
> >      spice_channel_connect(channel);
> >      g_ptr_array_add(self->priv->channels, channel);
> >  
> > +    g_signal_connect(channel, "channel-event", G_CALLBACK(channel_event),
> > self);
> > +
> >      spice_usb_device_manager_check_redir_on_connect(self, channel);
> >  
> >      /*
> > @@ -871,6 +875,33 @@ static void channel_destroy(SpiceSession *session,
> > SpiceChannel *channel,
> >      g_ptr_array_remove(self->priv->channels, channel);
> >  }
> >  
> > +static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> > +                          gpointer user_data)
> > +
> > +{
> > +    SpiceUsbDeviceManager *self = user_data;
> > +
> > +    switch (event) {
> > +    case SPICE_CHANNEL_NONE:
> > +    case SPICE_CHANNEL_OPENED:
> > +        return;
> > +
> > +    case SPICE_CHANNEL_SWITCHING:
> > +    case SPICE_CHANNEL_CLOSED:
> > +    case SPICE_CHANNEL_ERROR_CONNECT:
> > +        /* TODO: Maybe reconnect again */
> > +    case SPICE_CHANNEL_ERROR_TLS:
> > +    case SPICE_CHANNEL_ERROR_LINK:
> > +    case SPICE_CHANNEL_ERROR_AUTH:
> > +    case SPICE_CHANNEL_ERROR_IO:
> > +        g_signal_handlers_disconnect_by_func(channel, channel_event,
> > user_data);
> > +        g_ptr_array_remove(self->priv->channels, channel);
> > +        return;
> > +    default:
> > +        g_warn_if_reached();
> > +    }
> > +}
> > +
> >  static void spice_usb_device_manager_auto_connect_cb(GObject      *gobject,
> >                                                       GAsyncResult *res,
> >                                                       gpointer
> >                                                       user_data)
> 
> I don't know much the code but won't be safer instead of removing the
> invalid state object from the list instead not adding to the list on
> the first place and adding when the object is in the right state?

Changing to add object when channel it is already connected in
oppose to when channel is created (SpiceSession::channel-new)
would require more changes and careful thought as I'm not
familiar with this stack :(

This patch deals with the situation with the current codebase.

As there is ongoing refactor around this at [0] I'd recommend to
improve the code base at the same time.

[0] https://github.com/daynix/spice-gtk/commits/cd-linux

> Would not be better to rename spice_channel_connect to
> spice_channel_connect_async if this function is asynchronous?

Makes sense too...
-------------- 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/20180907/52779f3d/attachment.sig>