[PATCH wayland v5] client: extend error handling

Jason Ekstrand jason at jlekstrand.net
Tue Jul 22 19:44:00 PDT 2014


Sorry for not looking at it.  I'm mostly ok with the API so it's fine that
you pushed it.  I did have a question or two though.


On Mon, Jul 7, 2014 at 7:28 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> From: Marek Chalupa <mchqwerty at gmail.com>
>
> When an error occurs, wl_display_get_error() 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_protocol_error() DOES NOT indicate that 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.
>
> [Pekka Paalanen] Applied another hunk of Bryce's comments to docs,
>         added libtool version bump.
>
> Reviewed-by: Pekka Paalanen <ppaalanen at gmail.com>
> Reviewed-by: Bryce Harrington <b.harrington at samsung.com>
> ---
>  Makefile.am          |   2 +-
>  src/wayland-client.c | 137
> ++++++++++++++++++++++++++++++++++++++++++++-------
>  src/wayland-client.h |   3 ++
>  3 files changed, 123 insertions(+), 19 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index c15d8b8..fee19ab 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -51,7 +51,7 @@ nodist_libwayland_server_la_SOURCES =         \
>
>  libwayland_client_la_CFLAGS = $(FFI_CFLAGS) $(GCC_CFLAGS) -pthread
>  libwayland_client_la_LIBADD = $(FFI_LIBS) libwayland-util.la -lrt -lm
> -libwayland_client_la_LDFLAGS = -version-info 2:0:2
> +libwayland_client_la_LDFLAGS = -version-info 3:0:3
>  libwayland_client_la_SOURCES =                 \
>         src/wayland-client.c
>
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index e8aab7e..19b5694 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. */
> +               uint32_t code;
> +               /* interface (protocol) in which the error 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 */
> +               uint32_t id;
> +       } 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;
>
>         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 indicates that in some object an error occured.
> + * Difference between this function and display_fatal_error()
> + * is that this one handles errors that will come by 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, uint32_t code,
> +                      uint32_t id, const struct wl_interface *intf)
>  {
> +       struct wl_event_queue *iter;
> +       int err;
> +
> +       if (display->last_error)
> +               return;
> +
> +       /* 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;
> +       }
> +
>         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);
>

Why are you doing this?  Is someone waiting on you to display an error or
waiting for the error code?  I don't really see what there is to wake up
here.


> +
>         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);
>  }
>
>  static void
> @@ -1486,6 +1543,50 @@ wl_display_get_error(struct wl_display *display)
>         return ret;
>  }
>
> +/**
> + * Retrieves the information about a protocol error:
> + *
> + * \param display    The Wayland display
> + * \param interface  if not NULL, stores the interface where the error
> occurred
> + * \param id         if not NULL, stores the object id that generated
> + *                   the error. There's no guarantee the object is
> + *                   still valid; the client must know if it deleted the
> object.
>

Does the client even know what to do with the ID?  I mean, sure, they could
print it in an error message, but how useful is it?  I suppose they could
check to see if wl_proxy_get_id(my_proxy) == id to see if it happend at
that point in the stream.  Is this what you intend?


> + * \return           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 uint32_t
> +wl_display_get_protocol_error(struct wl_display *display,
> +                             const struct wl_interface **interface,
> +                             uint32_t *id)
> +{
> +       uint32_t 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..dc4df53 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);
> +uint32_t wl_display_get_protocol_error(struct wl_display *display,
> +                                      const struct wl_interface
> **interface,
> +                                      uint32_t *id);
>
>  int wl_display_flush(struct wl_display *display);
>  int wl_display_roundtrip(struct wl_display *display);
> --
> 1.8.5.5
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140722/13d84943/attachment-0001.html>


More information about the wayland-devel mailing list