[PATCH wayland v5] client: extend error handling
Pekka Paalanen
ppaalanen at gmail.com
Tue Jul 22 23:19:12 PDT 2014
On Tue, 22 Jul 2014 19:44:00 -0700
Jason Ekstrand <jason at jlekstrand.net> wrote:
> 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.
I originally had the same question. :-)
This is answered in
http://lists.freedesktop.org/archives/wayland-devel/2014-April/014539.html
Is that ok? There is also my reply to that:
http://lists.freedesktop.org/archives/wayland-devel/2014-April/014541.html
> > +
> > 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?
Pretty much, yeah. All this error reporting is pretty useless for
normal apps, but it should help developers, and especially allow
writing very explicit tests, where we can check that a case that should
lead to a protocol error actually does invoke the correct protocol
error referencing the correct object. Tests are the primary motivation
for this.
We use ID, because we cannot use the wl_proxy pointer, as the proxy
might not exist anymore.
Thanks,
pq
> > + * \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
> >
More information about the wayland-devel
mailing list