[Spice-devel] [PATCH spice-gtk 5/5] webdav: remove the client if input stream is closed

[Marc-André Lureau]

Hi

----- Original Message -----
> On Thu, Aug 17, 2017 at 03:35:57PM +0200, marcandre.lureau at redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > 
> > Instead of printing a warning when trying to read from a closed
> > stream. It can happen that libsoup closes the pipe output stream while
> > the input stream had no pending operation, in which case the close is
> > successful and we shouldn't try to read from the stream anymore.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > ---
> >  src/channel-webdav.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/channel-webdav.c b/src/channel-webdav.c
> > index 4a246b5..f1b6c2a 100644
> > --- a/src/channel-webdav.c
> > +++ b/src/channel-webdav.c
> > @@ -218,7 +218,7 @@ client_ref(Client *client)
> >      return client;
> >  }
> >  
> > -static void client_start_read(Client *client);
> > +static bool client_start_read(Client *client);
> >  
> >  static void remove_client(Client *client)
> >  {
> > @@ -234,10 +234,9 @@ static void mux_pushed_cb(OutputQueue *q, gpointer
> > user_data)
> >  {
> >      Client *client = user_data;
> >  
> > -    if (client->mux.size == 0) {
> > +    if (client->mux.size == 0 ||
> > +        !client_start_read(client)) {
> >          remove_client(client);
> > -    } else {
> > -        client_start_read(client);
> >      }
> >  
> >      client_unref(client);
> > @@ -280,14 +279,18 @@ end:
> >      client_unref(client);
> >  }
> >  
> > -static void client_start_read(Client *client)
> > +static bool client_start_read(Client *client)
> >  {
> >      GInputStream *input;
> >  
> >      input = g_io_stream_get_input_stream(G_IO_STREAM(client->pipe));
> > +    if (g_input_stream_is_closed(input)) {
> > +        return false;
> > +    }
> >      g_input_stream_read_async(input, client->mux.buf, MAX_MUX_SIZE,
> >                                G_PRIORITY_DEFAULT, client->cancellable,
> >                                server_reply_cb,
> >                                client_ref(client));
> > +    return true;
> >  }
> >  
> >  static void start_demux(SpiceWebdavChannel *self);
> > @@ -361,6 +364,7 @@ static void start_client(SpiceWebdavChannel *self)
> >      SoupServer *server;
> >      GSocketAddress *addr;
> >      GError *error = NULL;
> > +    bool started;
> >  
> >      session = spice_channel_get_session(SPICE_CHANNEL(self));
> >      server =
> >      phodav_server_get_soup_server(spice_session_get_webdav_server(session));
> > @@ -384,7 +388,8 @@ G_GNUC_END_IGNORE_DEPRECATIONS
> >  
> >      g_hash_table_insert(c->clients, &client->id, client);
> >  
> > -    client_start_read(client);
> > +    started = client_start_read(client);
> > +    g_assert(started);
> 
> Isn't it assert too much?

Since the pipe was just created for the client, it should never return false in client_start_read(), because g_input_stream_is_closed() is not possible. For me, assert() is appropriate in this case. I tend to keep return_if/warnings for unexpected errors that we can cope with, not for impossible situations (like failing to alloc, that glib assert on too for ex).

> 
> >      demux_to_client(client);
> >  
> >      g_clear_object(&addr);
> > --
> > 2.14.0.1.geff633fa0
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>