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

[Marc-André Lureau]

On Wed, Feb 12, 2014 at 1:12 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 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.

It is reset when the error is cleared. This is not the client to
bother when. However, the documentation could specify when the error
is set, for example in the channel-event signal doc, I'll update the
patch with that.

> 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.

I disagree to specify and constrain the usage of this function.

-- 
Marc-André Lureau