<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On 28 August 2014 17:52, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, 28 Aug 2014 15:59:37 +0200<br>
<div><div class="h5">Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> On 28 August 2014 12:02, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote:<br>
><br>
> > On Mon, 4 Aug 2014 11:30:46 +0200<br>
> > Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
> ><br>
> > > From the doc comment I get the feeling, that after successfull call to<br>
> > > wl_display_prepare_read(), the thread gains exclusive access to the fd.<br>
> > > That is not true. It only ensures that _one_ of the threads, that called<br>
> > > this function, will read from the fd and there will be no race.<br>
> > ><br>
> > > Here's slice of the commit message that introduces<br>
> > > wl_display_prepare_read() (3c7e8bfbb4745315b7bcbf69fa746c3d6718c305):<br>
> > ><br>
> > > "wl_display_prepare_read() registers the calling thread as a potential<br>
> > > reader of events. Once data is available on the fd, all reader<br>
> > > threads must call wl_display_read_events(), at which point<br>
> > > one of the threads will read from the fd and distribute the events<br>
> > > to event queues. When that is done, all threads return from<br>
> > > wl_display_read_events()."<br>
> > ><br>
> > > That is not clear from the doc message.<br>
> > > ---<br>
> > > src/wayland-client.c | 27 +++++++++++++++++++++++----<br>
> > > 1 file changed, 23 insertions(+), 4 deletions(-)<br>
> > ><br>
> > > diff --git a/src/wayland-client.c b/src/wayland-client.c<br>
> > > index 19b5694..d7de24d 100644<br>
> > > --- a/src/wayland-client.c<br>
> > > +++ b/src/wayland-client.c<br>
> > > @@ -1155,6 +1155,12 @@ read_events(struct wl_display *display)<br>
> > > pthread_cond_broadcast(&display->reader_cond);<br>
> > > } else {<br>
> > > serial = display->read_serial;<br>
> > > + /* in the case that the thread is not allowed to read<br>
> > > + * from the fd, make it sleep until reading and dispatching<br>
> > > + * is done. The condition display->read_serial == serial)<br>
> > is<br>
> > > + * because it may happen, that some thread will call this<br>
> > > + * function after the reading is done and without this, the<br>
> > > + * thread would block until next pthread_cond_broadcast */<br>
> > > while (display->read_serial == serial)<br>
> > > pthread_cond_wait(&display->reader_cond,<br>
> > > &display->mutex);<br>
> ><br>
> > I think the condition here is to ensure, that the function call will<br>
> > not return until someone has actually read the fd, not to avoid<br>
> > blocking in some cases. Apparently there could be spurious wake-ups,<br>
> > and need to protect against them. The 'serial' is set to<br>
> > display->read_serial just before waiting, and the mutex is held, so the<br>
> > cond_wait is guaranteed to happen at least once.<br>
> ><br>
> > This branch cannot be entered "after" the reading was done, because<br>
> > it is always the last call here that reads, when reader_count hits zero.<br>
> ><br>
> ><br>
> Yes, you're right. That's the real purpose of it. My mistake. However,<br>
> AFAIK in the current code this loop<br>
> will run only once, because everywhere is the cond broadcast called along<br>
> with<br>
> increasing the serial.<br>
<br>
</div></div>The manual I could find:<br>
<a href="http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_wait.html" target="_blank">http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_cond_wait.html</a><br>
says that spurious wakeups may occur. So better keep the serial<br>
there.<br>
<div class=""><br></div></blockquote><div> </div><div>Oh, didn't know that. Anyway, I wouldn't even think of removing the serial - it's working xD<br></div><div>and it is an assurance for future extensions.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
<br>
> Regarding the allowed actions.. do you have something particular in mind?<br>
> Theoretically,<br>
> programmer can safely use everything that doesn't change<br>
> display->readers_count.<br>
> (basically everything except wl_display_dispatch_queue).<br>
> But I don't know why he would do that - the expected action is just to call<br>
> wl_display_read_events<br>
> (with possibly poll before that) once you're prepared to read.<br>
<br>
</div>Nothing particular in my mind, I was just wondering if there was<br>
any old API that might break this. OTOH, that can be written off as<br>
just not using the new API correctly.<br>
<div class=""><br>
> > Or better yet, add a chapter to the publican docs about integrating<br>
> > libwayland-client to an event loop. I don't recall there being that<br>
> > yet...<br>
> ><br>
> ><br>
> > Thanks,<br>
> > pq<br>
> ><br>
><br>
> Thanks for notes,<br>
> I'll fix the patch and eventually try to write up some 'big doc' for that.<br>
<br>
</div>Very good. I think this is one of the areas that are easy to get<br>
subtly wrong, so a good doc is valuable. :-)<br>
<br></blockquote><div> </div><div>When I first ran into that, the word 'subtly' was not in place xD<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
pq<br></blockquote><div><br></div><div>Thanks,<br>Marek <br></div></div><br></div></div>