[Spice-devel] [PATCH spice-gtk 06/13] migration: set connecting state before fd request

[Marc-André Lureau]

----- Original Message -----
> On Sun, Nov 09, 2014 at 05:31:38PM +0100, Marc-André Lureau wrote:
> > During migration, the main channel initiating the process is waiting on
> > connection completion or error. However, if the migration is cancelled,
> > but the main channel state is still NONE, no error event will be fired,
> > and the main channel will remain frozen.
> 
> Do you mean SPICE_CHANNEL_STATE_UNCONNECTED rather than 'NONE' and are
> you referring to the
> if (c->state == SPICE_CHANNEL_STATE_UNCONNECTED)
>     return;
> 
> c->has_error = TRUE; /* break the loop */
> 
> blocks in channel_disconnect/spice_channel_disconnect() ?


This in spice_channel_disconnect():

   if (reason != SPICE_CHANNEL_NONE)
        g_signal_emit(G_OBJECT(channel), signals[SPICE_CHANNEL_EVENT], 0, reason);



> 
> > 
> > Setting connecting state before requesting the channel fd will emit an
> > error on disconnect, and cancel migration.
> > ---
> >  gtk/spice-channel.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> > index 93c2a78..9375c68 100644
> > --- a/gtk/spice-channel.c
> > +++ b/gtk/spice-channel.c
> > @@ -2461,10 +2461,8 @@ static gboolean channel_connect(SpiceChannel
> > *channel)
> >          g_warning("%s: channel setup incomplete", __FUNCTION__);
> >          return false;
> >      }
> > -    if (c->state != SPICE_CHANNEL_STATE_UNCONNECTED) {
> > -        g_warning("Invalid channel_connect state: %d", c->state);
> > -        return true;
> > -    }
> > +
> > +    c->state = SPICE_CHANNEL_STATE_CONNECTING;
> >  
> >      if (spice_session_get_client_provided_socket(c->session)) {
> >          if (c->fd == -1) {
> > @@ -2476,7 +2474,7 @@ static gboolean channel_connect(SpiceChannel
> > *channel)
> >              return true;
> >          }
> >      }
> > -    c->state = SPICE_CHANNEL_STATE_CONNECTING;
> > +
> >      c->xmit_queue_blocked = FALSE;
> >  
> >      g_return_val_if_fail(c->sock == NULL, FALSE);
> > @@ -2533,6 +2531,11 @@ gboolean spice_channel_open_fd(SpiceChannel
> > *channel, int fd)
> >      c = channel->priv;
> >      c->fd = fd;
> >  
> > +    if (c->state > SPICE_CHANNEL_STATE_CONNECTING) {
> > +        g_warning("Invalid channel_connect state: %d", c->state);
> > +        return true;
> > +    }
> > +
> 
> This hunk is in both spice_channel_connect() and spice_channel_open_fd()

It's not the same block.

In spice_channel_connect(), it returns TRUE if >= CONNECTING.

In spice_channel_open_fd(), it returns a TRUE, but warns if > CONNECTING. I am moving this condition as a pre-condition to not override existing fd in a seperate patch.


> (without the g_warning), I guess this could stay in channel_connect()
> and be removed from spice_channel_connect() ? Rereading the commit log,
> this movement seems unrelated to what you are fixing anyway?
> 
> >      return channel_connect(channel);
> >  }
> >  
> > --
> > 1.9.3
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>