On Fri, 18 Apr 2014 12:27:59 +0200 Marek Chalupa <mchqwe...@gmail.com> wrote:
> On 15 April 2014 15:36, Pekka Paalanen <ppaala...@gmail.com> wrote: > > > On Fri, 11 Apr 2014 11:39:13 +0200 > > Marek Chalupa <mchqwe...@gmail.com> 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 <ppaala...@gmail.com> 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. 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. > > 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. Yeah, I would not worry about it. > > > @@ -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->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; > > > + /* 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. 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. > 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 :) 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. > > > 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 */ > } Yeah, something like that. Thanks, pq _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel