[PATCH] client: extend error handling

Marek Chalupa mchqwerty at gmail.com
Wed Apr 30 00:40:19 PDT 2014


On 24 April 2014 14:45, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Wed, 23 Apr 2014 14:39:48 +0200
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > When an error occurres, than wl_display_get_error() do not
>
> ... occurs, [then] ... does 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.
>
> +o, a non-protocol error happened?
>
> > It returns valid information only in that case that (protocol) error
> > occurred, 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 | 138
> ++++++++++++++++++++++++++++++++++++++++++++-------
> >  src/wayland-client.h |   3 ++
> >  2 files changed, 123 insertions(+), 18 deletions(-)
> >
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index bd40313..f8146e7 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -78,7 +78,24 @@ struct wl_event_queue {
> >  struct wl_display {
> >       struct wl_proxy proxy;
> >       struct wl_connection *connection;
> > +
> > +     /* errno of the last wl_display error */
> >       int last_error;
> > +
> > +     /* When display gets an error event from some object, it stores
> > +      * information about it here, so that client can get this
> > +      * information afterwards */
> > +     struct {
> > +             /* Code of the error. It can be compared to
> > +              * the interface's errors enumeration. */
> > +             int code;
> > +             /* interface (protocol) in wich the error occured */
>
> which, occurred
>
> > +             const struct wl_interface *interface;
> > +             /* id of the proxy that caused the error. There's no
> warranty
> > +              * that the proxy is still valid. It's up to client how it
> will
> > +              * use it */
> > +             unsigned int id;
>
> I think we use uint32_t for ids specifically.
>

Yes, uint32_t, I'll fix that.


>
> > +     } protocol_error;
> >       int fd;
> >       pthread_t display_thread;
> >       struct wl_map objects;
> > @@ -96,6 +113,14 @@ struct wl_display {
> >
> >  static int debug_client = 0;
> >
> > +/**
> > + * This function is called for local errors (no memory, server hung up)
> > + *
> > + * \param display
> > + * \param error    error value (EINVAL, EFAULT, ...)
> > + *
> > + * \note this function is called with display mutex locked
> > + */
> >  static void
> >  display_fatal_error(struct wl_display *display, int error)
> >  {
> > @@ -105,7 +130,7 @@ display_fatal_error(struct wl_display *display, int
> error)
> >               return;
> >
> >       if (!error)
> > -             error = 1;
> > +             error = EFAULT;
>
> Yup, makes sense. Better EFAULT than EPERM for the case where errno is
> still 0, I guess. :-)
>
> >
> >       display->last_error = error;
> >
> > @@ -113,11 +138,56 @@ display_fatal_error(struct wl_display *display,
> int error)
> >               pthread_cond_broadcast(&iter->cond);
> >  }
> >
> > +/**
> > + * This function is called for error events
> > + * and idicates that in some object occured an error.
> > + * Difference between this function and display_fatal_error()
> > + * is that this one handles errors that will come in wire, whereas
> > + * display_fatal_error() is called for local errors.
> > + *
> > + * \param display
> > + * \param code    error code
> > + * \param id      id of the object that generated the error
> > + * \param intf    protocol interface
> > + */
> >  static void
> > -wl_display_fatal_error(struct wl_display *display, int error)
> > +display_protocol_error(struct wl_display *display, int code,
> > +                    unsigned int id, const struct wl_interface *intf)
> >  {
> > +     struct wl_event_queue *iter;
> > +     int err;
> > +
> > +     /* set correct errno */
> > +     if (wl_interface_equal(intf, &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;
> > +             }
> > +     } else {
> > +             err = EPROTO;
> > +     }
> > +
> > +     if (display->last_error)
> > +             return;
>
> This 'if' could be the very first thing. Doesn't matter.
>
> > +
> >       pthread_mutex_lock(&display->mutex);
> > -     display_fatal_error(display, error);
> > +
> > +     display->last_error = err;
> > +
> > +     display->protocol_error.code = code;
> > +     display->protocol_error.id = id;
> > +     display->protocol_error.interface = intf;
> > +
> > +     wl_list_for_each(iter, &display->event_queue_list, link)
> > +             pthread_cond_broadcast(&iter->cond);
>
> What is the purpose of this pthread_cond_broadcast()? I am not too
> familiar with the logic here, but looks like this is something new you
> added, and I'm not sure it belongs with the addition of
> wl_display_get_protocol_error().
>
> If this is fixing something, I feel like it should be a separate patch,
> since I can't see how adding wl_display_get_protocol_error() would add
> the need for this. Either the need was there to begin with and this
> bug fix should get into the stable releases, or it's not needed. Am I
> missing something?
>
>
This broadcast is not new, so I didn't touch it. So far as I see into it it
should
wake up all the sleeping event queues (when there's more of them than the
default one - in multi-threaded environment) so that they can receive the
error
and notify the client.

relevant commits:
    33b7637b4500a682018b503837b8aca9afae36f2
    385fe30e8b144a968aa88c6546c2ef247771b3d7

However, I don't see any pthread_cond_{wait, timedwait} for this condition
anywhere in the code and that's weird...


> > +
> >       pthread_mutex_unlock(&display->mutex);
> >  }
> >
> > @@ -579,25 +649,12 @@ display_handle_error(void *data,
> >                    uint32_t code, const char *message)
> >  {
> >       struct wl_proxy *proxy = object;
> > -     int err;
> >
> >       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;
> > -     }
> > -
> > -     wl_display_fatal_error(display, err);
> > +     display_protocol_error(display, code, proxy->object.id,
> > +                            proxy->object.interface);
>
> We are ignoring the 'message'. I would say that is on purpose and
> desireable, since we never standardise the error messages. If we
> exposed the error message to apps, someone might have the idea of
> checking it rather than only logging it. All ok here IMO.
>
> >  }
> >
> >  static void
> > @@ -1489,6 +1546,51 @@ wl_display_get_error(struct wl_display *display)
> >       return ret;
> >  }
> >
> > +/**
> > + * Retrieve the information about a protocol error
> > + *
> > + * \param display    display
> > + * \param interface  if not NULL, store there an interface on which the
> error
> > + *                   occured
> > + * \param id         if not NULL, store there the id of the object that
> generated
> > + *                   the error. There's no warranty that the object is
> still valid.
> > + *                   Client must know if he deleted the object or not.
> > + * \return           Code of the error
>
> The error code as defined in the interface specification.
>
> > + *
> > + * \code
> > + * int err = wl_display_get_error(display);
> > + *
> > + * if (err == EPROTO) {
> > + *        code = wl_display_get_protocol_error(display, &interface,
> &id);
> > + *        handle_error(code, interface, id);
> > + * }
> > + *
> > + * ...
> > + *
> > + *  \endcode
> > + */
> > +WL_EXPORT int
> > +wl_display_get_protocol_error(struct wl_display *display,
> > +                           const struct wl_interface **interface,
> > +                           unsigned int *id)
>
> uint32_t *id
>
> > +{
> > +     int ret;
> > +
> > +     pthread_mutex_lock(&display->mutex);
> > +
> > +     ret = display->protocol_error.code;
> > +
> > +     if (interface)
> > +             *interface = display->protocol_error.interface;
> > +     if (id)
> > +             *id = display->protocol_error.id;
> > +
> > +     pthread_mutex_unlock(&display->mutex);
> > +
> > +     return ret;
> > +}
> > +
> > +
> >  /** Send all buffered requests on the display to the server
> >   *
> >   * \param display The display context object
> > 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);
>
> uint32_t *id
>
> >
> >  int wl_display_flush(struct wl_display *display);
> >  int wl_display_roundtrip(struct wl_display *display);
>
> Looking good! Mostly just spelling to complain about. :-)
>

Yeah, the spelling.. I'll try to fix that xD


> The id type and the pthread_cond_broadcast need to be addressed still.
>
>
> Thanks,
> pq
>

Sorry for the delayed answer, I've been quite busy recently.

Thanks,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140430/cbd20127/attachment-0001.html>


More information about the wayland-devel mailing list