[PATCH wayland v5] client: extend error handling
Marek Chalupa
mchqwerty at gmail.com
Tue Jul 22 23:45:41 PDT 2014
Hi,
I checked e-mail before my leave to vacation and saw this message, so
here's a quick response:
The two questions answered Pekka (thanks).
More discussion about this pthread_cond broadcasting is here:
http://lists.freedesktop.org/archives/wayland-devel/2014-June/015632.html
http://lists.freedesktop.org/archives/wayland-devel/2014-July/016045.html
I've recently was looking into it and have some patches locally. I think
there's a bug and the broadcast
should be there, but for display->reader_cond. For this condition is waited
in read_events. When a thread is waiting for
an access to the display's fd, it would block after an error without this
broadcast.
I have some work in progress regarding this:
https://github.com/mchalupa/wayland/commits/threading
It's still in hard development, but seems working. I haven't got to some
extensive testing yet, though.
See you in two weeks :)
Regards,
Marek
On 23 July 2014 04:44, 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.
>
>
>> +
>> 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/20140723/488503db/attachment-0001.html>
More information about the wayland-devel
mailing list