[RCF wayland] client: extend error handling
Marek Chalupa
mchqwerty at gmail.com
Fri Apr 18 03:27:59 PDT 2014
On 15 April 2014 15:36, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri, 11 Apr 2014 11:39:13 +0200
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > When an error occures, than wl_display_get_error() do not
> > provide any way of getting know if it was a local error or if it was
> > an error event, respectively what object caused the error and what
> > the error was.
> >
> > This patch introduces a new function wl_display_get_protocol_error()
> > which will return error code, interface and id of the object that
> > generated the error.
> > wl_display_get_error() will work the same way as before.
> >
> > wl_display_get_protcol_error() DO NOT indicate that the error happed.
> > It returns valid information only in that case that (protocol) error
> > happend, so it should be used after calling wl_display_get_error()
> > with positive result.
> >
> > Thanks to Pekka Paalanen <ppaalanen at gmail.com> for pointing out
> > issues in the first versions of this patch.
> > ---
> > src/wayland-client.c | 149
> +++++++++++++++++++++++++++++++++++++++++++++------
> > src/wayland-client.h | 3 ++
> > 2 files changed, 136 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index bd40313..7f21fcd 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -78,7 +78,25 @@ struct wl_event_queue {
> > struct wl_display {
> > struct wl_proxy proxy;
> > struct wl_connection *connection;
> > +
> > + /* errno of the last wl_display error or 1 when the error
> > + * came via wire as an event */
>
> 1 is EPERM, we might just pick an errno that fits best, and call it
> that.
>
>
True, 1 is not a right choice. All errnos are positive, so what about to
use -1? Then the wl_display_get_error() would return the same as before for
local errors and -1 for protocol errors.
> But does the code really do this?
>
> I suppose we are still free to pick the errno, since the implementation
> so far has been buggy, interpreting all protocol error codes in the
> wl_display context and picking the errno based on that, instead
> accounting for the actual interface it came with.
>
The current implementation is buggy, but what if some old code relies on
currently picked errnos? On the other hand, I think that in most cases it
does not, so it wouldn't do much damage to pick errno freely.
>
> int last_error;
> > +
> > + /* When display gets an error event from some object, it stores
> > + * its credentials here, so that client can get detailed
> information
>
> Would s/credentials/signature/ be more appropriate?
>
> > + * about the error afterwards */
> > + struct {
> > + /* Code of the error. It can be compared to
> > + * the interface's errors enumeration. */
> > + int code;
> > + /* interface (protocol) in wich the error occured */
> > + const struct wl_interface *interface;
> > + /* id of the proxy that caused the error. There's no
> warranty
> > + * that the proxy is still valid. It's up to client how it
> will
> > + * use it */
> > + unsigned int id;
> > + } protocol_error;
>
> Yup, that is probably all the info we can give or might need.
>
> > int fd;
> > pthread_t display_thread;
> > struct wl_map objects;
> > @@ -96,6 +114,14 @@ struct wl_display {
> >
> > static int debug_client = 0;
> >
> > +/**
> > + * This function is called for local errors (no memory, server hung up)
> > + *
> > + * \param display
> > + * \param error error value (EINVAL, EFAULT, ...)
> > + *
> > + * \note this function is called with display mutex locked
> > + */
> > static void
> > display_fatal_error(struct wl_display *display, int error)
> > {
> > @@ -105,7 +131,7 @@ display_fatal_error(struct wl_display *display, int
> error)
> > return;
> >
> > if (!error)
> > - error = 1;
> > + error = EFAULT;
> >
> > display->last_error = error;
> >
> > @@ -113,11 +139,49 @@ display_fatal_error(struct wl_display *display,
> int error)
> > pthread_cond_broadcast(&iter->cond);
> > }
> >
> > +/**
> > + * This function is called for error events
> > + * and idicates that in some object occured an error.
> > + * Difference between this function and display_fatal_error()
> > + * is that this one handles errors that will come in wire, whereas
> > + * display_fatal_error() is called for local errors.
> > + *
> > + * \param display
> > + * \param code error code
> > + * \param id id of the object that generated the error
> > + * \param intf protocol interface
> > + * \param err errno for compatibility of wl_display_get_error() or 1
> > + */
> > static void
> > -wl_display_fatal_error(struct wl_display *display, int error)
> > +display_protocol_error(struct wl_display *display, int code,
> > + unsigned int id, const struct wl_interface *intf,
> > + int err)
> > {
> > + struct wl_event_queue *iter;
> > +
> > + /* error should be a number from an enumeration */
> > + assert(code >= 0);
>
> 'code' comes from the wire, right?
> Then this assert should not be here, because even if the server sends
> something bad, it's no reason to abort when we are going to shut down
> anyway.
>
> An error within an error is no reason to quit delivering the original
> error if possible. ;-)
>
Holy true.
>
> > + /* it's either 1 or ESOMETHING */
> > + assert(err >= 0);
> > +
> > + if (display->last_error)
> > + return;
> > +
> > pthread_mutex_lock(&display->mutex);
> > - display_fatal_error(display, error);
> > +
> > + /* Since wl_display_get_error() returns errno,
> > + * we must set it here too for the sake of
> > + * compatibility (and, of course, for prevention of
> > + * multiple broadcasting iter->cond) */
> > + display->last_error = err;
> > +
> > + display->protocol_error.code = code;
> > + display->protocol_error.id = id;
> > + display->protocol_error.interface = intf;
> > +
> > + wl_list_for_each(iter, &display->event_queue_list, link)
> > + pthread_cond_broadcast(&iter->cond);
> > +
> > pthread_mutex_unlock(&display->mutex);
> > }
> >
> > @@ -579,25 +643,32 @@ display_handle_error(void *data,
> > uint32_t code, const char *message)
> > {
> > struct wl_proxy *proxy = object;
> > - int err;
> > + int err = 1;
> >
> > wl_log("%s@%u: error %d: %s\n",
> > proxy->object.interface->name, proxy->object.id, code,
> message);
> >
> > - switch (code) {
> > - case WL_DISPLAY_ERROR_INVALID_OBJECT:
> > - case WL_DISPLAY_ERROR_INVALID_METHOD:
> > - err = EINVAL;
> > - break;
> > - case WL_DISPLAY_ERROR_NO_MEMORY:
> > - err = ENOMEM;
> > - break;
> > - default:
> > - err = EFAULT;
> > - break;
> > + /* due to compatibility, we must pass an errno to
> display_protocol_error()
> > + * (for setting display->last_error). I can imagine that later
> this code
> > + * is deleted and protocol errors will have display->last_error
> set to 1
> > + */
> > + if (wl_interface_equal(proxy->object.interface,
> &wl_display_interface)) {
> > + switch (code) {
> > + case WL_DISPLAY_ERROR_INVALID_OBJECT:
> > + case WL_DISPLAY_ERROR_INVALID_METHOD:
> > + err = EINVAL;
> > + break;
> > + case WL_DISPLAY_ERROR_NO_MEMORY:
> > + err = ENOMEM;
> > + break;
> > + default:
> > + err = EFAULT;
> > + break;
> > + }
>
> This is what referred to at the top, we still set the errno as before
> instead of 1, right?
>
>
Yes, but now only for wl_display object.
I think we need to keep this, so we don't change the
> behaviour in the valid use case of wl_display_get_error().
>
>
Yes, that's the point of this part of code. Anyway, it would be nice to do
not do that, because it
causes inconsistency in identifying protocol vs. local errors. I hope it
could be removed some day.
Actually, if we would be picking new errnos, as you wrote before, then I
don't see any reason for this
code to be here any more. Once you change the numbers that will
wl_display_get_error() return,
let's do it properly :)
> }
> >
> > - wl_display_fatal_error(display, err);
> > + display_protocol_error(display, code, proxy->object.id,
> > + proxy->object.interface, err);
> > }
> >
> > static void
> > @@ -1489,6 +1560,52 @@ wl_display_get_error(struct wl_display *display)
> > return ret;
> > }
> >
> > +/**
> > + * Retrieve the information about an protocol error
> > + *
> > + * If no protocol error occured, then the interface returned is NULL
> and id is 0.
> > + * First you sould call wl_display_get_error() to find out if an error
> occured
> > + * and then you can check if it was local error or protocol error.
>
> Usually we find out about errors by a libwayland-client function
> returning failure, and then wl_display_get_error() just provides
> additional information, contrary to something like glGetError().
>
> > + *
> > + * \param display display proxy
>
> It's not a wl_proxy but the real thing.
>
> > + * \param interface if not NULL, store there and interface on which
> the error
> > + * occured
> > + * \param id if not NULL, store there the id of the object that
> generated
> > + * the error. There's no warranty that the object is
> still valid.
> > + * Client must know if he deleted the object or not.
> > + * \return Code of the error
> > + *
> > + * \code
> > + * int err = wl_display_get_error(display);
> > + *
> > + * if (err) {
> > + * code = wl_display_get_protocol_error(display, &interface,
> &id);
> > + * handle_error(code, interface, id);
> > + * }
> > + * \endcode
> > + */
> > +WL_EXPORT int
> > +wl_display_get_protocol_error(struct wl_display *display,
> > + const struct wl_interface **interface,
> > + unsigned int *id)
> > +{
> > + int ret;
> > +
> > + pthread_mutex_lock(&display->mutex);
> > +
> > + ret = display->protocol_error.code;
> > +
> > + if (interface)
> > + *interface = display->protocol_error.interface;
> > + if (id)
> > + *id = display->protocol_error.id;
> > +
> > + pthread_mutex_unlock(&display->mutex);
> > +
> > + return ret;
> > +}
> > +
> > +
> > /** Send all buffered requests on the display to the server
> > *
> > * \param display The display context object
> > diff --git a/src/wayland-client.h b/src/wayland-client.h
> > index 2a32785..61aec0c 100644
> > --- a/src/wayland-client.h
> > +++ b/src/wayland-client.h
> > @@ -161,6 +161,9 @@ int wl_display_dispatch_queue_pending(struct
> wl_display *display,
> > struct wl_event_queue *queue);
> > int wl_display_dispatch_pending(struct wl_display *display);
> > int wl_display_get_error(struct wl_display *display);
> > +int wl_display_get_protocol_error(struct wl_display *display,
> > + const struct wl_interface **interface,
> > + unsigned int *id);
> >
> > int wl_display_flush(struct wl_display *display);
> > int wl_display_roundtrip(struct wl_display *display);
>
> Right, I like this idea, not only because it was something I wished
> for. :-)
>
> I read through this patch, but didn't have time to do an in-depth
> review of the correctness yet.
>
>
> Thanks,
> pq
>
Thanks for comments. To sum it up, I'd like to do it like:
wl_display_get_error() would return 0 when no error occurred, errno for
local errors (that's the same as now) and -1 for protocol errors. Then you
could use it like:
err = wl_display_get_error(display);
if (err == -1) {
wl_display_get_protocol_error(....);
... /* handle protocol errors */
} else if (err) {
... /* handle errors */
}
Thanks,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140418/1ea40a41/attachment-0001.html>
More information about the wayland-devel
mailing list