<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On 24 April 2014 14:45, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="">On Wed, 23 Apr 2014 14:39:48 +0200<br>
Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> When an error occurres, than wl_display_get_error() do not<br>
<br>
</div>... occurs, [then] ... does not<br>
<div class=""><br>
> provide any way of getting know if it was a local error or if it was<br>
> an error event, respectively what object caused the error and what<br>
> the error was.<br>
><br>
> This patch introduces a new function wl_display_get_protocol_error()<br>
> which will return error code, interface and id of the object that<br>
> generated the error.<br>
> wl_display_get_error() will work the same way as before.<br>
><br>
> wl_display_get_protcol_error() DO NOT indicate that the error happed.<br>
<br>
</div>+o, a non-protocol error happened?<br>
<div><div class="h5"><br>
> It returns valid information only in that case that (protocol) error<br>
> occurred, so it should be used after calling wl_display_get_error()<br>
> with positive result.<br>
><br>
> Thanks to Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> for pointing out<br>
> issues in the first versions of this patch.<br>
> ---<br>
> src/wayland-client.c | 138 ++++++++++++++++++++++++++++++++++++++++++++-------<br>
> src/wayland-client.h | 3 ++<br>
> 2 files changed, 123 insertions(+), 18 deletions(-)<br>
><br>
> diff --git a/src/wayland-client.c b/src/wayland-client.c<br>
> index bd40313..f8146e7 100644<br>
> --- a/src/wayland-client.c<br>
> +++ b/src/wayland-client.c<br>
> @@ -78,7 +78,24 @@ struct wl_event_queue {<br>
> struct wl_display {<br>
> struct wl_proxy proxy;<br>
> struct wl_connection *connection;<br>
> +<br>
> + /* errno of the last wl_display error */<br>
> int last_error;<br>
> +<br>
> + /* When display gets an error event from some object, it stores<br>
> + * information about it here, so that client can get this<br>
> + * information afterwards */<br>
> + struct {<br>
> + /* Code of the error. It can be compared to<br>
> + * the interface's errors enumeration. */<br>
> + int code;<br>
> + /* interface (protocol) in wich the error occured */<br>
<br>
</div></div>which, occurred<br>
<div class=""><br>
> + const struct wl_interface *interface;<br>
> + /* id of the proxy that caused the error. There's no warranty<br>
> + * that the proxy is still valid. It's up to client how it will<br>
> + * use it */<br>
> + unsigned int id;<br>
<br>
</div>I think we use uint32_t for ids specifically.<br></blockquote><div><br></div><div>Yes, uint32_t, I'll fix that. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class=""><br>
> + } protocol_error;<br>
> int fd;<br>
> pthread_t display_thread;<br>
> struct wl_map objects;<br>
> @@ -96,6 +113,14 @@ struct wl_display {<br>
><br>
> static int debug_client = 0;<br>
><br>
> +/**<br>
> + * This function is called for local errors (no memory, server hung up)<br>
> + *<br>
> + * \param display<br>
> + * \param error error value (EINVAL, EFAULT, ...)<br>
> + *<br>
> + * \note this function is called with display mutex locked<br>
> + */<br>
> static void<br>
> display_fatal_error(struct wl_display *display, int error)<br>
> {<br>
> @@ -105,7 +130,7 @@ display_fatal_error(struct wl_display *display, int error)<br>
> return;<br>
><br>
> if (!error)<br>
> - error = 1;<br>
> + error = EFAULT;<br>
<br>
</div>Yup, makes sense. Better EFAULT than EPERM for the case where errno is<br>
still 0, I guess. :-)<br>
<div><div class="h5"><br>
><br>
> display->last_error = error;<br>
><br>
> @@ -113,11 +138,56 @@ display_fatal_error(struct wl_display *display, int error)<br>
> pthread_cond_broadcast(&iter->cond);<br>
> }<br>
><br>
> +/**<br>
> + * This function is called for error events<br>
> + * and idicates that in some object occured an error.<br>
> + * Difference between this function and display_fatal_error()<br>
> + * is that this one handles errors that will come in wire, whereas<br>
> + * display_fatal_error() is called for local errors.<br>
> + *<br>
> + * \param display<br>
> + * \param code error code<br>
> + * \param id id of the object that generated the error<br>
> + * \param intf protocol interface<br>
> + */<br>
> static void<br>
> -wl_display_fatal_error(struct wl_display *display, int error)<br>
> +display_protocol_error(struct wl_display *display, int code,<br>
> + unsigned int id, const struct wl_interface *intf)<br>
> {<br>
> + struct wl_event_queue *iter;<br>
> + int err;<br>
> +<br>
> + /* set correct errno */<br>
> + if (wl_interface_equal(intf, &wl_display_interface)) {<br>
> + switch (code) {<br>
> + case WL_DISPLAY_ERROR_INVALID_OBJECT:<br>
> + case WL_DISPLAY_ERROR_INVALID_METHOD:<br>
> + err = EINVAL;<br>
> + break;<br>
> + case WL_DISPLAY_ERROR_NO_MEMORY:<br>
> + err = ENOMEM;<br>
> + break;<br>
> + default:<br>
> + err = EFAULT;<br>
> + }<br>
> + } else {<br>
> + err = EPROTO;<br>
> + }<br>
> +<br>
> + if (display->last_error)<br>
> + return;<br>
<br>
</div></div>This 'if' could be the very first thing. Doesn't matter.<br>
<div class=""><br>
> +<br>
> pthread_mutex_lock(&display->mutex);<br>
> - display_fatal_error(display, error);<br>
> +<br>
> + display->last_error = err;<br>
> +<br>
> + display->protocol_error.code = code;<br>
> + display-><a href="http://protocol_error.id" target="_blank">protocol_error.id</a> = id;<br>
> + display->protocol_error.interface = intf;<br>
> +<br>
> + wl_list_for_each(iter, &display->event_queue_list, link)<br>
> + pthread_cond_broadcast(&iter->cond);<br>
<br>
</div>What is the purpose of this pthread_cond_broadcast()? I am not too<br>
familiar with the logic here, but looks like this is something new you<br>
added, and I'm not sure it belongs with the addition of<br>
wl_display_get_protocol_error().<br>
<br>
If this is fixing something, I feel like it should be a separate patch,<br>
since I can't see how adding wl_display_get_protocol_error() would add<br>
the need for this. Either the need was there to begin with and this<br>
bug fix should get into the stable releases, or it's not needed. Am I<br>
missing something?<br>
<div class=""><br></div></blockquote><div><br></div><div>This broadcast is not new, so I didn't touch it. So far as I see into it it should<br></div><div>wake up all the sleeping event queues (when there's more of them than the<br>
default one - in multi-threaded environment) so that they can receive the error<br>and notify the client.<br><br>relevant commits:<br> 33b7637b4500a682018b503837b8aca9afae36f2<br> 385fe30e8b144a968aa88c6546c2ef247771b3d7<br>
<br></div><div>However, I don't see any pthread_cond_{wait, timedwait} for this condition<br></div><div>anywhere in the code and that's weird...<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="">
> +<br>
> pthread_mutex_unlock(&display->mutex);<br>
> }<br>
><br>
> @@ -579,25 +649,12 @@ display_handle_error(void *data,<br>
> uint32_t code, const char *message)<br>
> {<br>
> struct wl_proxy *proxy = object;<br>
> - int err;<br>
><br>
> wl_log("%s@%u: error %d: %s\n",<br>
> proxy->object.interface->name, proxy-><a href="http://object.id" target="_blank">object.id</a>, code, message);<br>
><br>
> - switch (code) {<br>
> - case WL_DISPLAY_ERROR_INVALID_OBJECT:<br>
> - case WL_DISPLAY_ERROR_INVALID_METHOD:<br>
> - err = EINVAL;<br>
> - break;<br>
> - case WL_DISPLAY_ERROR_NO_MEMORY:<br>
> - err = ENOMEM;<br>
> - break;<br>
> - default:<br>
> - err = EFAULT;<br>
> - break;<br>
> - }<br>
> -<br>
> - wl_display_fatal_error(display, err);<br>
> + display_protocol_error(display, code, proxy-><a href="http://object.id" target="_blank">object.id</a>,<br>
> + proxy->object.interface);<br>
<br>
</div>We are ignoring the 'message'. I would say that is on purpose and<br>
desireable, since we never standardise the error messages. If we<br>
exposed the error message to apps, someone might have the idea of<br>
checking it rather than only logging it. All ok here IMO.<br>
<div class=""><br>
> }<br>
><br>
> static void<br>
> @@ -1489,6 +1546,51 @@ wl_display_get_error(struct wl_display *display)<br>
> return ret;<br>
> }<br>
><br>
> +/**<br>
> + * Retrieve the information about a protocol error<br>
> + *<br>
> + * \param display display<br>
> + * \param interface if not NULL, store there an interface on which the error<br>
> + * occured<br>
> + * \param id if not NULL, store there the id of the object that generated<br>
> + * the error. There's no warranty that the object is still valid.<br>
> + * Client must know if he deleted the object or not.<br>
> + * \return Code of the error<br>
<br>
</div>The error code as defined in the interface specification.<br>
<div class=""><br>
> + *<br>
> + * \code<br>
> + * int err = wl_display_get_error(display);<br>
> + *<br>
> + * if (err == EPROTO) {<br>
> + * code = wl_display_get_protocol_error(display, &interface, &id);<br>
> + * handle_error(code, interface, id);<br>
> + * }<br>
> + *<br>
> + * ...<br>
> + *<br>
> + * \endcode<br>
> + */<br>
> +WL_EXPORT int<br>
> +wl_display_get_protocol_error(struct wl_display *display,<br>
> + const struct wl_interface **interface,<br>
> + unsigned int *id)<br>
<br>
</div>uint32_t *id<br>
<div><div class="h5"><br>
> +{<br>
> + int ret;<br>
> +<br>
> + pthread_mutex_lock(&display->mutex);<br>
> +<br>
> + ret = display->protocol_error.code;<br>
> +<br>
> + if (interface)<br>
> + *interface = display->protocol_error.interface;<br>
> + if (id)<br>
> + *id = display-><a href="http://protocol_error.id" target="_blank">protocol_error.id</a>;<br>
> +<br>
> + pthread_mutex_unlock(&display->mutex);<br>
> +<br>
> + return ret;<br>
> +}<br>
> +<br>
> +<br>
> /** Send all buffered requests on the display to the server<br>
> *<br>
> * \param display The display context object<br>
> diff --git a/src/wayland-client.h b/src/wayland-client.h<br>
> index 2a32785..61aec0c 100644<br>
> --- a/src/wayland-client.h<br>
> +++ b/src/wayland-client.h<br>
> @@ -161,6 +161,9 @@ int wl_display_dispatch_queue_pending(struct wl_display *display,<br>
> struct wl_event_queue *queue);<br>
> int wl_display_dispatch_pending(struct wl_display *display);<br>
> int wl_display_get_error(struct wl_display *display);<br>
> +int wl_display_get_protocol_error(struct wl_display *display,<br>
> + const struct wl_interface **interface,<br>
> + unsigned int *id);<br>
<br>
</div></div>uint32_t *id<br>
<div class=""><br>
><br>
> int wl_display_flush(struct wl_display *display);<br>
> int wl_display_roundtrip(struct wl_display *display);<br>
<br>
</div>Looking good! Mostly just spelling to complain about. :-)<br></blockquote><div><br></div><div>Yeah, the spelling.. I'll try to fix that xD<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The id type and the pthread_cond_broadcast need to be addressed still.<br>
<br>
<br>
Thanks,<br>
pq<br>
</blockquote></div><br></div><div class="gmail_extra">Sorry for the delayed answer, I've been quite busy recently.<br><br></div><div class="gmail_extra">Thanks,<br>Marek<br></div></div>