<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On 22 August 2014 13:01, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, 22 Aug 2014 12:52:09 +0200 <div><div class="h5">Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote: > On 22 August 2014 12:40, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote: > > > On Tue, 5 Aug 2014 11:43:35 +0200 > > Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> 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. </div></div>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? </blockquote><div> </div><div>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 </div><div>is not usually performed with the mutex locked IIRC. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Thanks, pq </blockquote></div> </div><div class="gmail_extra">Thanks, Marek </div></div>