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

[Marek Chalupa]

On 22 August 2014 13:01, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> 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?
>
>
Yes, you're right. I'll make new version of the patch.

Well, I think I should check for this race all over the code, since this
check
is not usually performed with the mutex locked IIRC.


>
> Thanks,
> pq
>

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