<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On 19 April 2014 10:22, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 18 Apr 2014 12:27:59 +0200 <div><div class="h5">Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote: > On 15 April 2014 15:36, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote: > > > On Fri, 11 Apr 2014 11:39:13 +0200 > > Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote: > > > > > When an error occures, than wl_display_get_error() do 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_protcol_error() DO NOT indicate that the error happed. > > > It returns valid information only in that case that (protocol) error > > > happend, so it should be used after calling wl_display_get_error() > > > with positive result. > > > > > > Thanks to Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> for pointing out > > > issues in the first versions of this patch. > > > --- > > > src/wayland-client.c | 149 > > +++++++++++++++++++++++++++++++++++++++++++++------ > > > src/wayland-client.h | 3 ++ > > > 2 files changed, 136 insertions(+), 16 deletions(-) > > > > > > diff --git a/src/wayland-client.c b/src/wayland-client.c > > > index bd40313..7f21fcd 100644 > > > --- a/src/wayland-client.c > > > +++ b/src/wayland-client.c > > > @@ -78,7 +78,25 @@ struct wl_event_queue { > > > struct wl_display { > > > struct wl_proxy proxy; > > > struct wl_connection *connection; > > > + > > > + /* errno of the last wl_display error or 1 when the error > > > + * came via wire as an event */ > > > > 1 is EPERM, we might just pick an errno that fits best, and call it > > that. > > > > > 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. </div></div>Looking at the manual page of errno, it doesn't explicitly require errno to be non-negative, so I guess -1 would be ok. It seems POSIX.1 defines EPROTO, maybe what would be approriate for custom (non-wl_display) protocol errors instead? It would make strerror() spew something more understandable. </blockquote><div> </div><div>EPROTO sounds good to me. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class=""> > > But does the code really do this? > > > > I suppose we are still free to pick the errno, since the implementation > > so far has been buggy, interpreting all protocol error codes in the > > wl_display context and picking the errno based on that, instead > > accounting for the actual interface it came with. > > > > 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. </div>Yeah, I would not worry about it. <div><div class="h5"> > > > @@ -579,25 +643,32 @@ display_handle_error(void *data, > > > uint32_t code, const char *message) > > > { > > > struct wl_proxy *proxy = object; > > > - int err; > > > + int err = 1; > > > > > > wl_log("%s@%u: error %d: %s\n", > > > proxy->object.interface->name, proxy-><a href="http://object.id" target="_blank">object.id</a>, 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; > > > + /* due to compatibility, we must pass an errno to > > display_protocol_error() > > > + * (for setting display->last_error). I can imagine that later > > this code > > > + * is deleted and protocol errors will have display->last_error > > set to 1 > > > + */ > > > + if (wl_interface_equal(proxy->object.interface, > > &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; > > > + break; > > > + } > > > > This is what referred to at the top, we still set the errno as before > > instead of 1, right? > > > > > Yes, but now only for wl_display object. > > I think we need to keep this, so we don't change the > > behaviour in the valid use case of wl_display_get_error(). > > > > > Yes, that's the point of this part of code. Anyway, it would be nice to do > not do that, because it > causes inconsistency in identifying protocol vs. local errors. I hope it > could be removed some day. </div></div>I'm not sure. Protocol errors on wl_display are sort of special, like generic "out of memory" that is not tied to any specific protocol extension. I'd say keep the behaviour for wl_display, and change it for everything else. <div class=""> </div></blockquote><div> </div><div>I'm totally OK with ENOMEM, it's reasonable - once you're out of memory, it doesn't matter where the error occurred. But when we will set EINVAL for the </div><div>invalid object/method (protocol errors) then it will make it harder to </div><div>tell off these errors from local errors, i. e. EINVAL from recvmsg. I'd rather set EPROTO for these two as well. However, I don't see it as a big deal and I'm OK with keeping these errnos as it is, so I'm sending new version of a patch. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class=""> > Actually, if we would be picking new errnos, as you wrote before, then I > don't see any reason for this > code to be here any more. Once you change the numbers that will > wl_display_get_error() return, > let's do it properly :) </div>That's what I am aiming for. Let's keep the return codes that make sense, and fix the rest. So we'd be picking a new errno only for non-wl_display error events. Up to now, they have been practically undefined. <div><div class="h5"> > > > diff --git a/src/wayland-client.h b/src/wayland-client.h > > > index 2a32785..61aec0c 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); > > > +int wl_display_get_protocol_error(struct wl_display *display, > > > + const struct wl_interface **interface, > > > + unsigned int *id); > > > > > > int wl_display_flush(struct wl_display *display); > > > int wl_display_roundtrip(struct wl_display *display); > > > > Right, I like this idea, not only because it was something I wished > > for. :-) > > > > I read through this patch, but didn't have time to do an in-depth > > review of the correctness yet. > > > > > > Thanks, > > pq > > > > Thanks for comments. To sum it up, I'd like to do it like: > > 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: > > err = wl_display_get_error(display); > > if (err == -1) { > wl_display_get_protocol_error(....); > ... /* handle protocol errors */ > } else if (err) { > ... /* handle errors */ > } </div></div>Yeah, something like that. Thanks, pq </blockquote></div> </div></div>