[RCF wayland] client: extend error handling
Marek Chalupa
mchqwerty at gmail.com
Wed Apr 23 05:33:09 PDT 2014
On 19 April 2014 10:22, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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.
>
EPROTO sounds good to me.
> > > 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.
>
>
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
invalid object/method (protocol errors) then it will make it harder to
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.
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140423/69f85493/attachment.html>
More information about the wayland-devel
mailing list