[PATCH] client: extend error handling

[Pekka Paalanen]

On Wed, 30 Apr 2014 09:40:19 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:

> 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:
> >
> > > +/**
> > > + * 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

I was confused about it because your patch never removes any lines
doing pthread_cond_broadcast(), but it does add it.

Ok, now I see it. Originally, display_fatal_error() does it, and
wl_display_fatal_error() called it. Your patch renames
wl_display_fatal_error to display_protocol_error and removes the call
to display_fatal_error(), so you copy back the relevant parts of
display_fatal_error().

> 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...

Marking the 'cond' member deprecated, the compiler tells me it is used
in the following places in current master:

  CC       src/libwayland_client_la-wayland-client.lo
src/wayland-client.c: In function 'display_fatal_error':
src/wayland-client.c:113:3: warning: 'cond' is deprecated (declared at src/wayland-client.c:75) [-Wdeprecated-declarations]
** pthread_cond_broadcast(&iter->cond);

src/wayland-client.c: In function 'wl_event_queue_init':
src/wayland-client.c:128:2: warning: 'cond' is deprecated (declared at src/wayland-client.c:75) [-Wdeprecated-declarations]
** pthread_cond_init(&queue->cond, NULL);

src/wayland-client.c: In function 'wl_event_queue_release':
src/wayland-client.c:143:2: warning: 'cond' is deprecated (declared at src/wayland-client.c:75) [-Wdeprecated-declarations]
** pthread_cond_destroy(&queue->cond);

src/wayland-client.c: In function 'queue_event':
src/wayland-client.c:979:3: warning: 'cond' is deprecated (declared at src/wayland-client.c:75) [-Wdeprecated-declarations]
** pthread_cond_signal(&queue->cond);

You're right, and I cannot see a related mutex, either. Maybe it is
unneeded or we have a hard to trigger bug here.

Nice find.


Thanks,
pq