[PATCH wayland 4/8] client: Move prepare read documentation to .._prepare_read_queue()
Jonas Ådahl
jadahl at gmail.com
Wed Oct 7 21:53:57 PDT 2015
On Fri, Oct 02, 2015 at 01:32:21PM -0700, Bryce Harrington wrote:
> On Fri, Oct 02, 2015 at 05:32:55PM +0800, Jonas Ådahl wrote:
> > In the documentation we refer to "an event queue" in various places and
> > from the beginning it is unclear what event queue this means. So,
> > instead of having a paragraph in the end mentioning this, move the
> > detailed documentation to the function with the queue explicitly passed.
> >
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> > src/wayland-client.c | 132 ++++++++++++++++++++++++---------------------------
> > 1 file changed, 62 insertions(+), 70 deletions(-)
> >
> > diff --git a/src/wayland-client.c b/src/wayland-client.c
> > index ed33886..056d341 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -1372,67 +1372,31 @@ err:
> > return -1;
> > }
> >
> > -/** Prepare to read events from the display to this queue
> > +/** Prepare to read events from the display's file descriptor to a queue
> > *
> > * \param display The display context object
> > * \param queue The event queue to use
> > * \return 0 on success or -1 if event queue was not empty
> > *
> > - * Atomically makes sure the queue is empty and stops any other thread
> > - * from placing events into this (or any) queue. Caller must
> > - * eventually call either wl_display_cancel_read() or
> > - * wl_display_read_events(), usually after waiting for the
> > - * display fd to become ready for reading, to release the lock.
> > + * This function (or wl_display_prepare_read()) must be called before reading
> > + * from the file descriptor using wl_display_read_events(). Calling
> > + * wl_display_prepare_read_queue() announces the calling thread's intention to
> > + * read and ensures that until the thread is ready to read and calls
> > + * wl_display_read_events(), no other thread will read from the file descriptor.
> > + * This only succeeds if the event queue is empty, and if not -1 is returned and
> > + * errno set to EAGAIN.
> > *
> > - * \sa wl_display_prepare_read
> > - * \memberof wl_event_queue
> > - */
> > -WL_EXPORT int
> > -wl_display_prepare_read_queue(struct wl_display *display,
> > - struct wl_event_queue *queue)
> > -{
> > - int ret;
> > -
> > - pthread_mutex_lock(&display->mutex);
> > -
> > - if (!wl_list_empty(&queue->event_list)) {
> > - errno = EAGAIN;
> > - ret = -1;
> > - } else {
> > - display->reader_count++;
> > - ret = 0;
> > - }
> > -
> > - pthread_mutex_unlock(&display->mutex);
> > -
> > - return ret;
> > -}
> > -
> > -/** Prepare to read events from the display's file descriptor
> > - *
> > - * \param display The display context object
> > - * \return 0 on success or -1 if event queue was not empty
> > + * If a thread successfully calls wl_display_prepare_read_queue(), it must
> > + * either call wl_display_read_events() when it's ready or cancel the read
> > + * intention by calling wl_display_cancel_read().
> > *
> > - * This function must be called before reading from the file
> > - * descriptor using wl_display_read_events(). Calling
> > - * wl_display_prepare_read() announces the calling thread's intention
> > - * to read and ensures that until the thread is ready to read and
> > - * calls wl_display_read_events(), no other thread will read from the
> > - * file descriptor. This only succeeds if the event queue is empty
> > - * though, and if there are undispatched events in the queue, -1 is
> > - * returned and errno set to EAGAIN.
> > - *
> > - * If a thread successfully calls wl_display_prepare_read(), it must
> > - * either call wl_display_read_events() when it's ready or cancel the
> > - * read intention by calling wl_display_cancel_read().
> > - *
> > - * Use this function before polling on the display fd or to integrate
> > - * the fd into a toolkit event loop in a race-free way.
> > - * A correct usage would be (we left out most of error checking):
> > + * Use this function before polling on the display fd or integrate the fd into a
> > + * toolkit event loop in a race-free way. A correct usage would be (with most
> > + * error checking left out):
> > *
> > * \code
> > - * while (wl_display_prepare_read(display) != 0)
> > - * wl_display_dispatch_pending(display);
> > + * while (wl_display_prepare_read_queue(display, queue) != 0)
> > + * wl_display_dispatch_queue_pending(display, queue);
> > * wl_display_flush(display);
> > *
> > * ret = poll(fds, nfds, -1);
> > @@ -1441,14 +1405,14 @@ wl_display_prepare_read_queue(struct wl_display *display,
> > * else
> > * wl_display_read_events(display);
> > *
> > - * wl_display_dispatch_pending(display);
> > + * wl_display_dispatch_queue_pending(display, queue);
> > * \endcode
> > *
> > - * Here we call wl_display_prepare_read(), which ensures that between
> > - * returning from that call and eventually calling
> > - * wl_display_read_events(), no other thread will read from the fd and
> > - * queue events in our queue. If the call to wl_display_prepare_read() fails,
> > - * we dispatch the pending events and try again until we're successful.
> > + * Here we call wl_display_prepare_read_queue(), which ensures that between
> > + * returning from that call and eventually calling wl_display_read_events(), no
> > + * other thread will read from the fd and queue events in our queue. If the call
> > + * to wl_display_prepare_read_queue() fails, we dispatch the pending events and
> > + * try again until we're successful.
> > *
> > * When using wl_display_dispatch() we'd have something like:
> > *
> > @@ -1472,23 +1436,51 @@ wl_display_prepare_read_queue(struct wl_display *display,
> > * before the other thread managed to call poll(), it will
> > * block with events queued.
> > *
> > - * wl_display_prepare_read() function doesn't acquire exclusive access
> > - * to the display's fd. It only registers that the thread calling this function
> > - * has intention to read from fd.
> > - * When all registered readers call wl_display_read_events(),
> > - * only one (at random) eventually reads and queues the events and the
> > - * others are sleeping meanwhile. This way we avoid races and still
> > - * can read from more threads.
> > + * wl_display_prepare_read_queue() function doesn't acquire exclusive access to
> > + * the display's fd. It only registers that the thread calling this function has
> > + * has intention to read from fd. When all registered readers call
>
> 'has has' here.
>
> Also, to follow suit with your other improvements, maybe tack a 'The' on
> the front of this paragraph while you're at it?
>
> > + * wl_display_read_events(), only one (at random) eventually reads and queues
> > + * the events and the others are sleeping meanwhile. This way we avoid races and
> > + * still can read from more threads.
> > *
> > - * If the relevant queue is not the default queue, then
> > - * wl_display_prepare_read_queue() and wl_display_dispatch_queue_pending()
> > - * need to be used instead.
> > - *
> > - * \sa wl_display_cancel_read(), wl_display_read_events()
> > + * \sa wl_display_cancel_read(), wl_display_read_events(),
> > + * wl_display_prepare_read()
> > *
> > * \memberof wl_display
> > */
> > WL_EXPORT int
> > +wl_display_prepare_read_queue(struct wl_display *display,
> > + struct wl_event_queue *queue)
> > +{
> > + int ret;
> > +
> > + pthread_mutex_lock(&display->mutex);
> > +
> > + if (!wl_list_empty(&queue->event_list)) {
> > + errno = EAGAIN;
> > + ret = -1;
> > + } else {
> > + display->reader_count++;
> > + ret = 0;
> > + }
> > +
> > + pthread_mutex_unlock(&display->mutex);
> > +
> > + return ret;
> > +}
> > +
> > +/** Prepare to read events from the display's file descriptor
> > + *
> > + * \param display The display context object
> > + * \return 0 on success or -1 if event queue was not empty
> > + *
> > + * This function does the same thing as wl_display_prepare_read_queue()
> > + * with the default queue passed as the queue.
> > + *
> > + * \sa wl_display_prepare_read_queue
> > + * \memberof wl_event_queue
> > + */
> > +WL_EXPORT int
> > wl_display_prepare_read(struct wl_display *display)
> > {
> > return wl_display_prepare_read_queue(display, &display->default_queue);
>
> Apart from the two grammar quibbles, this document refactor LGTM.
>
> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
Thanks! I fixed the grammar nits you had and pushed it:
To ssh://jadahl@git.freedesktop.org/git/wayland/wayland
b6809e5..7c9135e master -> master
Jonas
>
> > --
> > 2.4.3
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list