[PATCH] client: check for error before prepare/read

[Pekka Paalanen]

On Fri, 22 Aug 2014 12:52:09 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:

> On 22 August 2014 12:40, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 
> > On Tue,  5 Aug 2014 11:43:35 +0200
> > Marek Chalupa <mchqwerty at gmail.com> wrote:
> >
> > > This prevents from blocking shown in one display test. Also, it
> > > makes sense to not proceed further in the code of these function
> > > when an error occurred.
> > > ---
> > >  src/wayland-client.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > > index 68a91f6..f176dd4 100644
> > > --- a/src/wayland-client.c
> > > +++ b/src/wayland-client.c
> > > @@ -1183,6 +1183,9 @@ wl_display_read_events(struct wl_display *display)
> > >  {
> > >       int ret;
> > >
> > > +     if (display->last_error)
> > > +             return -1;
> >
> > The documentation requires that errno is set. We definitely need to set
> > it, as errno is thread-local. If we are on a thread where
> > libwayland-client already set errno, we should still overwrite it to
> > say that there already was an error.
> >
> 
> 
> > > +
> > >       pthread_mutex_lock(&display->mutex);
> > >
> > >       ret = read_events(display);
> > > @@ -1229,6 +1232,9 @@ wl_display_prepare_read_queue(struct wl_display
> > *display,
> > >  {
> > >       int ret;
> > >
> > > +     if (display->last_error)
> > > +             return -1;
> >
> > This will need a documentation update for wl_display_prepare_read(),
> > that -1 can be returned if the wl_display is already in an error state.
> > It should also set errno.
> >
> >
> Ok
> 
> 
> > > +
> > >       pthread_mutex_lock(&display->mutex);
> > >
> > >       if (!wl_list_empty(&queue->event_list)) {
> >
> > What errno should we use?
> >
> > EDEADLK, because this is preventing a deadlock?
> > EIO, because... nothing really says "existing error"?
> > EACCESS, EPERM, because you're not supposed to do this?
> > EBUSY, because the wl_display has gone to lunch?
> > ECANCELED, because we cancelled your attempt for your own good?
> >
> > "Operation canceled", hmm, I think I like that one. ECANCELED ok?
> >
> >
> Hmm, yes, "Operation canceled" feels good for this case.
> 
> 
> > Also, it seems last_error should always be handled with the mutex
> > locked.
> >
> >
> No, when you're not writing to it, it don't need mutex locked. Once
> last_error is set, it is not modified
> anymore. At least not in the current code, so the mutex is not necessary I
> think.

You assume it has already been set.

But what if you are racing: you just checked and last_error is zero,
but before you can grab the mutex, some other thread caused an error,
and the result of the check just became invalid?


Thanks,
pq