[PATCH] client: extend error handling
Pekka Paalanen
ppaalanen at gmail.com
Thu Apr 24 05:45:05 PDT 2014
On Wed, 23 Apr 2014 14:39:48 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:
> When an error occurres, than wl_display_get_error() do not
... occurs, [then] ... does 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.
+o, a non-protocol error happened?
> It returns valid information only in that case that (protocol) error
> occurred, 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 | 138 ++++++++++++++++++++++++++++++++++++++++++++-------
> src/wayland-client.h | 3 ++
> 2 files changed, 123 insertions(+), 18 deletions(-)
>
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index bd40313..f8146e7 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -78,7 +78,24 @@ struct wl_event_queue {
> struct wl_display {
> struct wl_proxy proxy;
> struct wl_connection *connection;
> +
> + /* errno of the last wl_display error */
> int last_error;
> +
> + /* When display gets an error event from some object, it stores
> + * information about it here, so that client can get this
> + * information 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 */
which, occurred
> + 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;
I think we use uint32_t for ids specifically.
> + } protocol_error;
> int fd;
> pthread_t display_thread;
> struct wl_map objects;
> @@ -96,6 +113,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 +130,7 @@ display_fatal_error(struct wl_display *display, int error)
> return;
>
> if (!error)
> - error = 1;
> + error = EFAULT;
Yup, makes sense. Better EFAULT than EPERM for the case where errno is
still 0, I guess. :-)
>
> display->last_error = error;
>
> @@ -113,11 +138,56 @@ 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
> + */
> 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)
> {
> + struct wl_event_queue *iter;
> + int err;
> +
> + /* set correct errno */
> + if (wl_interface_equal(intf, &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;
> + }
> + } else {
> + err = EPROTO;
> + }
> +
> + if (display->last_error)
> + return;
This 'if' could be the very first thing. Doesn't matter.
> +
> pthread_mutex_lock(&display->mutex);
> - display_fatal_error(display, error);
> +
> + 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);
What is the purpose of this pthread_cond_broadcast()? I am not too
familiar with the logic here, but looks like this is something new you
added, and I'm not sure it belongs with the addition of
wl_display_get_protocol_error().
If this is fixing something, I feel like it should be a separate patch,
since I can't see how adding wl_display_get_protocol_error() would add
the need for this. Either the need was there to begin with and this
bug fix should get into the stable releases, or it's not needed. Am I
missing something?
> +
> pthread_mutex_unlock(&display->mutex);
> }
>
> @@ -579,25 +649,12 @@ display_handle_error(void *data,
> uint32_t code, const char *message)
> {
> struct wl_proxy *proxy = object;
> - int err;
>
> 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;
> - }
> -
> - wl_display_fatal_error(display, err);
> + display_protocol_error(display, code, proxy->object.id,
> + proxy->object.interface);
We are ignoring the 'message'. I would say that is on purpose and
desireable, since we never standardise the error messages. If we
exposed the error message to apps, someone might have the idea of
checking it rather than only logging it. All ok here IMO.
> }
>
> static void
> @@ -1489,6 +1546,51 @@ wl_display_get_error(struct wl_display *display)
> return ret;
> }
>
> +/**
> + * Retrieve the information about a protocol error
> + *
> + * \param display display
> + * \param interface if not NULL, store there an 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
The error code as defined in the interface specification.
> + *
> + * \code
> + * int err = wl_display_get_error(display);
> + *
> + * if (err == EPROTO) {
> + * 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)
uint32_t *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);
uint32_t *id
>
> int wl_display_flush(struct wl_display *display);
> int wl_display_roundtrip(struct wl_display *display);
Looking good! Mostly just spelling to complain about. :-)
The id type and the pthread_cond_broadcast need to be addressed still.
Thanks,
pq
More information about the wayland-devel
mailing list