<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 15 April 2014 15:36, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, 11 Apr 2014 11:39:13 +0200<br>
Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> When an error occures, than wl_display_get_error() do not<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>
> It returns valid information only in that case that (protocol) error<br>
> happend, 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 | 149 +++++++++++++++++++++++++++++++++++++++++++++------<br>
> src/wayland-client.h | 3 ++<br>
> 2 files changed, 136 insertions(+), 16 deletions(-)<br>
><br>
> diff --git a/src/wayland-client.c b/src/wayland-client.c<br>
> index bd40313..7f21fcd 100644<br>
> --- a/src/wayland-client.c<br>
> +++ b/src/wayland-client.c<br>
> @@ -78,7 +78,25 @@ 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 or 1 when the error<br>
> + * came via wire as an event */<br>
<br>
</div></div>1 is EPERM, we might just pick an errno that fits best, and call it<br>
that.<br>
<br></blockquote><div> </div><div>True, 1 is not a right choice. All errnos are positive, so what about to use -1? Then the wl_display_get_error() would return the same as before for local errors and -1 for protocol errors.<br>
</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But does the code really do this?<br>
<br>
I suppose we are still free to pick the errno, since the implementation<br>
so far has been buggy, interpreting all protocol error codes in the<br>
wl_display context and picking the errno based on that, instead<br>
accounting for the actual interface it came with.<br>
<div></div></blockquote><div><br></div><div>The current implementation is buggy, but what if some old code relies on currently picked errnos? On the other hand, I think that in most cases it does not, so it wouldn't do much damage to pick errno freely.<br>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div> <br></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">
> int last_error;<br>
> +<br>
> + /* When display gets an error event from some object, it stores<br>
> + * its credentials here, so that client can get detailed information<br>
<br>
</div>Would s/credentials/signature/ be more appropriate?<br></blockquote><div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> + * about the error 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>
> + 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>
> + } protocol_error;<br>
<br>
</div>Yup, that is probably all the info we can give or might need.<br>
<div><div class="h5"><br>
> int fd;<br>
> pthread_t display_thread;<br>
> struct wl_map objects;<br>
> @@ -96,6 +114,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 +131,7 @@ display_fatal_error(struct wl_display *display, int error)<br>
> return;<br>
><br>
> if (!error)<br>
> - error = 1;<br>
> + error = EFAULT;<br>
><br>
> display->last_error = error;<br>
><br>
> @@ -113,11 +139,49 @@ 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>
> + * \param err errno for compatibility of wl_display_get_error() or 1<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>
> + int err)<br>
> {<br>
> + struct wl_event_queue *iter;<br>
> +<br>
> + /* error should be a number from an enumeration */<br>
> + assert(code >= 0);<br>
<br>
</div></div>'code' comes from the wire, right?<br>
Then this assert should not be here, because even if the server sends<br>
something bad, it's no reason to abort when we are going to shut down<br>
anyway.<br>
<br>
An error within an error is no reason to quit delivering the original<br>
error if possible. ;-)<br></blockquote><div><br></div><div>Holy true.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> + /* it's either 1 or ESOMETHING */<br>
> + assert(err >= 0);<br>
> +<br>
> + if (display->last_error)<br>
> + return;<br>
> +<br>
> pthread_mutex_lock(&display->mutex);<br>
> - display_fatal_error(display, error);<br>
> +<br>
> + /* Since wl_display_get_error() returns errno,<br>
> + * we must set it here too for the sake of<br>
> + * compatibility (and, of course, for prevention of<br>
> + * multiple broadcasting iter->cond) */<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>
> pthread_mutex_unlock(&display->mutex);<br>
> }<br>
><br>
> @@ -579,25 +643,32 @@ display_handle_error(void *data,<br>
> uint32_t code, const char *message)<br>
> {<br>
> struct wl_proxy *proxy = object;<br>
> - int err;<br>
> + int err = 1;<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>
> + /* due to compatibility, we must pass an errno to display_protocol_error()<br>
> + * (for setting display->last_error). I can imagine that later this code<br>
> + * is deleted and protocol errors will have display->last_error set to 1<br>
> + */<br>
> + if (wl_interface_equal(proxy->object.interface, &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>
> + break;<br>
> + }<br>
<br>
</div></div>This is what referred to at the top, we still set the errno as before<br>
instead of 1, right?<br>
<br></blockquote><div> </div><div>Yes, but now only for wl_display object. <br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think we need to keep this, so we don't change the<br>
behaviour in the valid use case of wl_display_get_error().<br>
<div class=""><br></div></blockquote><div> </div><div>Yes, that's the point of this part of code. Anyway, it would be nice to do not do that, because it<br>causes inconsistency in identifying protocol vs. local errors. I hope it could be removed some day.<br>
<br></div><div>Actually, if we would be picking new errnos, as you wrote before, then I don't see any reason for this<br>code to be here any more. Once you change the numbers that will wl_display_get_error() return,<br>
let's do it properly :)<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> }<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, err);<br>
> }<br>
><br>
> static void<br>
> @@ -1489,6 +1560,52 @@ wl_display_get_error(struct wl_display *display)<br>
> return ret;<br>
> }<br>
><br>
> +/**<br>
> + * Retrieve the information about an protocol error<br>
> + *<br>
> + * If no protocol error occured, then the interface returned is NULL and id is 0.<br>
> + * First you sould call wl_display_get_error() to find out if an error occured<br>
> + * and then you can check if it was local error or protocol error.<br>
<br>
</div>Usually we find out about errors by a libwayland-client function<br>
returning failure, and then wl_display_get_error() just provides<br>
additional information, contrary to something like glGetError().<br>
<div class=""><br>
> + *<br>
> + * \param display display proxy<br>
<br>
</div>It's not a wl_proxy but the real thing. <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> + * \param interface if not NULL, store there and 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>
> + * \code<br>
> + * int err = wl_display_get_error(display);<br>
> + *<br>
> + * if (err) {<br>
> + * code = wl_display_get_protocol_error(display, &interface, &id);<br>
> + * handle_error(code, interface, id);<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>
> + 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>
> int wl_display_flush(struct wl_display *display);<br>
> int wl_display_roundtrip(struct wl_display *display);<br>
<br>
</div></div>Right, I like this idea, not only because it was something I wished<br>
for. :-)<br>
<br>
I read through this patch, but didn't have time to do an in-depth<br>
review of the correctness yet.<br>
<br>
<br>
Thanks,<br>
pq<br></blockquote><div><br></div><div>Thanks for comments. To sum it up, I'd like to do it like:<br><br></div><div>wl_display_get_error() would return 0 when no error occurred, errno for local errors (that's the same as now) and -1 for protocol errors. Then you could use it like:<br>
<br></div><div>err = wl_display_get_error(display);<br><br></div><div>if (err == -1) {<br></div><div> wl_display_get_protocol_error(....);<br></div><div> ... /* handle protocol errors */<br></div><div>} else if (err) {<br>
</div><div> ... /* handle errors */<br></div><div>}<br></div><div><br></div><div>Thanks,<br></div><div>Marek<br></div></div><br></div></div>