[PATCH wayland 4/8] client: Move prepare read documentation to .._prepare_read_queue()

[Bryce Harrington]

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>

> -- 
> 2.4.3
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel