[RCF wayland] client: extend error handling
Pekka Paalanen
ppaalanen at gmail.com
Sat Apr 19 01:22:50 PDT 2014
On Fri, 18 Apr 2014 12:27:59 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:
> On 15 April 2014 15:36, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> > On Fri, 11 Apr 2014 11:39:13 +0200
> > Marek Chalupa <mchqwerty at 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 <ppaalanen at 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
More information about the wayland-devel
mailing list