[Spice-devel] [spice-gtk 12/13] channel: add spice_channel_get_error()

[Christophe Fergeau]

On Tue, Feb 11, 2014 at 01:08:55PM -0500, Marc-André Lureau wrote:
> 
> 
> ----- Original Message -----
> > On Tue, Feb 11, 2014 at 06:50:18PM +0100, Marc-André Lureau wrote:
> > > >> @@ -2248,15 +2269,16 @@ static void *spice_channel_coroutine(void *data)
> > > >>
> > > >>
> > > >>  reconnect:
> > > >> -    c->conn = spice_session_channel_open_host(c->session, channel,
> > > >> &c->tls);
> > > >> +    c->conn = spice_session_channel_open_host(c->session, channel,
> > > >> &c->tls, &c->error);
> > > >>      if (c->conn == NULL) {
> > > >> -        if (!c->tls) {
> > > >> +        if (!c->error && !c->tls) {
> > > >>              CHANNEL_DEBUG(channel, "trying with TLS port");
> > > >>              c->tls = true; /* FIXME: does that really work with
> > > >>              provided fd */
> > > >>              goto reconnect;
> > > >>          } else {
> > > >>              CHANNEL_DEBUG(channel, "Connect error");
> > > >>              emit_main_context(channel, SPICE_CHANNEL_EVENT,
> > > >>              SPICE_CHANNEL_ERROR_CONNECT);
> > > >> +            g_clear_error(&c->error);
> > > >
> > > > My understanding of this bit is that spice_channel_get_error() can only
> > > > be
> > > > used in a callback attached to SPICE_CHANNEL_EVENT as after that it will
> > > > be
> > > > cleared? Maybe this needs to be more explicit in the API doc for
> > > > spice_channel_get_error().
> > > 
> > > It is meant to be usable at any point.

Currently, if you don't use it in this callback, all you will get is a NULL
error even if a connection error occurred before.

> > 
> > Won't the g_clear_error() I quoted cause spice_channel_get_error() to
> > return the correct error in the SPICE_CHANNEL_EVENT callback when there is
> > a connection error, and then return NULL after the g_clear_error() has been
> > called? Ie the error state of the channel will not be permanent after an
> > error has happened, but will be silently reset by the library.
> > Not saying this is bad per-se, at first I was worried that we could get in
> > a permanent error state it's not possible to get out of.
> 
> The API doesn't specify "when" the channel is in error state. When it is,
> you may get more information via this API, I don't see the need to make
> it more specific.

IThis does not strike me as a very good public API to export. From the
library user point of view, there will be this function that can give more
information about channel errors, but when an error occurred, the function
may return NULL when you call it as the library resets the error at
totally unspecified moments.
So it can be good enough as something internal, but imo we need to do
better at least in the API doc if it's something that we export.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20140212/0fe0fac0/attachment.pgp>