Re: [PATCH wayland v6] util: Document wl_list methods

[Bryce Harrington]

On Thu, Sep 22, 2016 at 09:59:37PM -0500, Yong Bakos wrote:
> From: Yong Bakos 
> 
> Add doxygen comment blocks to all wl_list methods.
> 
> Signed-off-by: Yong Bakos 

Reviewed-by: Bryce Harrington 

One extremely minor wording suggestion below only if you do another rev.
But this all looks extremely good; will be nice to get the wl_list
documentation shaped up, thanks.

Bryce

> ---
> v6: Change description to doubly-linked list (pq)
> v5: Change description to linked-list [err]
> Clarify uses of `wl_list_init` (pq)
> v4: Fix variable name in sample code. (pq)
> Remove implemenetation details of pointer state. (pq)
> Remove note about __typeof__ entirely.
> - it's not helpful as a note or a code comment either
> Change sample code indentation to just spaces. (pq)
> Use _list suffix for list in sample code. (pq)
> Use 'iterate' instead of enumerate, for consistency. (pq)
> Note that only removing 'pos' is safe for *_safe methods. (pq, giucam)
> v3: Standardize on term 'element', to match param names, tests, and other 
> text.
> Use 'relates' for macros (instead of 'memberof').
> v2: Refine the writing for clarity.
> Add dox for wl_list macros, omitted in v1.
> Add notices for unsafe operations and invalid states (giucam, pq)
>  src/wayland-util.h | 224 
> +++--
>  1 file changed, 184 insertions(+), 40 deletions(-)
> 
> diff --git a/src/wayland-util.h b/src/wayland-util.h
> index cacc122..71c26a1 100644
> --- a/src/wayland-util.h
> +++ b/src/wayland-util.h
> @@ -78,73 +78,150 @@ struct wl_interface {
> 
>  /** \class wl_list
>   *
> - * \brief doubly-linked list
> + * \brief Doubly-linked list
>   *
> - * The list head is of "struct wl_list" type, and must be initialized
> - * using wl_list_init().  All entries in the list must be of the same
> - * type.  The item type must have a "struct wl_list" member. This
> - * member will be initialized by wl_list_insert(). There is no need to
> - * call wl_list_init() on the individual item. To query if the list is
> - * empty in O(1), use wl_list_empty().
> + * On its own, an instance of `struct wl_list` represents the sentinel head 
> of
> + * a doubly-linked list, and must be initialized using wl_list_init().
> + * When empty, the list head's `next` and `prev` members point to the list 
> head
> + * itself, otherwise `next` references the first element in the list, and 
> `prev`
> + * refers to the last element in the list.
>   *
> - * Let's call the list reference "struct wl_list foo_list", the item type as
> - * "item_t", and the item member as "struct wl_list link".
> + * Use the `struct wl_list` type to represent both the list head and the 
> links
> + * between elements within the list. Use wl_list_empty() to determine if the
> + * list is empty in O(1).
> + *
> + * All elements in the list must be of the same type. The element type must 
> have
> + * a `struct wl_list` member, often named `link` by convention. Prior to
> + * insertion, there is no need to initialize an element's `link` - invoking
> + * wl_list_init() on an individual list element's `struct wl_list` member is
> + * unnecessary if the very next operation is wl_list_insert(). However, a
> + * common idiom is to initialize an element's `link` prior to removal - 
> ensure
> + * safety by invoking wl_list_init() before wl_list_remove().
> + *
> + * Consider a list reference `struct wl_list foo_list`, an element type as
> + * `struct element`, and an element's link member as `struct wl_list link`.
> + *
> + * The following code initializes a list and adds three elements to it.
>   *
> - * The following code will initialize a list:
>   * \code
>   * struct wl_list foo_list;
>   *
> - * struct item_t {
> - *   int foo;
> - *   struct wl_list link;
> + * struct element {
> + * int foo;
> + * struct wl_list link;
>   * };
> - * struct item_t item1, item2, item3;
> + * struct element e1, e2, e3;
>   *
>   * wl_list_init(_list);
> - * wl_list_insert(_list, );   // Pushes item1 at the head
> - * wl_list_insert(_list, );   // Pushes item2 at the head
> - * wl_list_insert(, ); // Pushes item3 after item2
> + * wl_list_insert(_list, );   // e1 is the first element
> + * wl_list_insert(_list, );   // e2 is now the first element
> + * wl_list_insert(, ); // insert e3 after e2
>   * \endcode
>
> - * The list now looks like [item2, item3, item1]
> + * The list now looks like [e2, e3, e1].
> + *
> + * The `wl_list` API provides some iterator macros. For example, to iterate
> + * a list in ascending order:
>   *
> - * Iterate the list in ascending order:
>   * \code
> - * item_t *item;
> - * wl_list_for_each(item, foo_list, link) {
> - *   Do_something_with_item(item);
> + * struct element *e;
> + * wl_list_for_each(e, foo_list, link) {
> + * do_something_with_element(e);
>   * }
>   * \endcode
> + *
> + * See the 

[PATCH wayland v6] util: Document wl_list methods

[Yong Bakos]

From: Yong Bakos 

Add doxygen comment blocks to all wl_list methods.

Signed-off-by: Yong Bakos 
---
v6: Change description to doubly-linked list (pq)
v5: Change description to linked-list [err]
Clarify uses of `wl_list_init` (pq)
v4: Fix variable name in sample code. (pq)
Remove implemenetation details of pointer state. (pq)
Remove note about __typeof__ entirely.
- it's not helpful as a note or a code comment either
Change sample code indentation to just spaces. (pq)
Use _list suffix for list in sample code. (pq)
Use 'iterate' instead of enumerate, for consistency. (pq)
Note that only removing 'pos' is safe for *_safe methods. (pq, giucam)
v3: Standardize on term 'element', to match param names, tests, and other text.
Use 'relates' for macros (instead of 'memberof').
v2: Refine the writing for clarity.
Add dox for wl_list macros, omitted in v1.
Add notices for unsafe operations and invalid states (giucam, pq)
 src/wayland-util.h | 224 +++--
 1 file changed, 184 insertions(+), 40 deletions(-)

diff --git a/src/wayland-util.h b/src/wayland-util.h
index cacc122..71c26a1 100644
--- a/src/wayland-util.h
+++ b/src/wayland-util.h
@@ -78,73 +78,150 @@ struct wl_interface {

 /** \class wl_list
  *
- * \brief doubly-linked list
+ * \brief Doubly-linked list
  *
- * The list head is of "struct wl_list" type, and must be initialized
- * using wl_list_init().  All entries in the list must be of the same
- * type.  The item type must have a "struct wl_list" member. This
- * member will be initialized by wl_list_insert(). There is no need to
- * call wl_list_init() on the individual item. To query if the list is
- * empty in O(1), use wl_list_empty().
+ * On its own, an instance of `struct wl_list` represents the sentinel head of
+ * a doubly-linked list, and must be initialized using wl_list_init().
+ * When empty, the list head's `next` and `prev` members point to the list head
+ * itself, otherwise `next` references the first element in the list, and 
`prev`
+ * refers to the last element in the list.
  *
- * Let's call the list reference "struct wl_list foo_list", the item type as
- * "item_t", and the item member as "struct wl_list link".
+ * Use the `struct wl_list` type to represent both the list head and the links
+ * between elements within the list. Use wl_list_empty() to determine if the
+ * list is empty in O(1).
+ *
+ * All elements in the list must be of the same type. The element type must 
have
+ * a `struct wl_list` member, often named `link` by convention. Prior to
+ * insertion, there is no need to initialize an element's `link` - invoking
+ * wl_list_init() on an individual list element's `struct wl_list` member is
+ * unnecessary if the very next operation is wl_list_insert(). However, a
+ * common idiom is to initialize an element's `link` prior to removal - ensure
+ * safety by invoking wl_list_init() before wl_list_remove().
+ *
+ * Consider a list reference `struct wl_list foo_list`, an element type as
+ * `struct element`, and an element's link member as `struct wl_list link`.
+ *
+ * The following code initializes a list and adds three elements to it.
  *
- * The following code will initialize a list:
  * \code
  * struct wl_list foo_list;
  *
- * struct item_t {
- * int foo;
- * struct wl_list link;
+ * struct element {
+ * int foo;
+ * struct wl_list link;
  * };
- * struct item_t item1, item2, item3;
+ * struct element e1, e2, e3;
  *
  * wl_list_init(_list);
- * wl_list_insert(_list, ); // Pushes item1 at the head
- * wl_list_insert(_list, ); // Pushes item2 at the head
- * wl_list_insert(, );   // Pushes item3 after item2
+ * wl_list_insert(_list, );   // e1 is the first element
+ * wl_list_insert(_list, );   // e2 is now the first element
+ * wl_list_insert(, ); // insert e3 after e2
  * \endcode
  *
- * The list now looks like [item2, item3, item1]
+ * The list now looks like [e2, e3, e1].
+ *
+ * The `wl_list` API provides some iterator macros. For example, to iterate
+ * a list in ascending order:
  *
- * Iterate the list in ascending order:
  * \code
- * item_t *item;
- * wl_list_for_each(item, foo_list, link) {
- * Do_something_with_item(item);
+ * struct element *e;
+ * wl_list_for_each(e, foo_list, link) {
+ * do_something_with_element(e);
  * }
  * \endcode
+ *
+ * See the documentation of each iterator for details.
+ * \sa 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/list.h
  */
 struct wl_list {
struct wl_list *prev;
struct wl_list *next;
 };

+/**
+ * Initializes the list.
+ *
+ * \param list List to initialize
+ *
+ * \memberof wl_list
+ */
 void
 wl_list_init(struct wl_list *list);

+/**
+ * Inserts an element into the list, after the element represented by \p list.
+ * When \p list is a reference to the list itself (the head), set 

[PATCH wayland v5] util: Document wl_list methods

[Yong Bakos]

From: Yong Bakos 

Add doxygen comment blocks to all wl_list methods.

Signed-off-by: Yong Bakos 
---
v5: Change description to linked-list (pq)
Clarify uses of `wl_list_init` (pq)
v4: Fix variable name in sample code. (pq)
Remove implemenetation details of pointer state. (pq)
Remove note about __typeof__ entirely.
- it's not helpful as a note or a code comment either
Change sample code indentation to just spaces. (pq)
Use _list suffix for list in sample code. (pq)
Use 'iterate' instead of enumerate, for consistency. (pq)
Note that only removing 'pos' is safe for *_safe methods. (pq, giucam)
v3: Standardize on term 'element', to match param names, tests, and other text.
Use 'relates' for macros (instead of 'memberof').
v2: Refine the writing for clarity.
Add dox for wl_list macros, omitted in v1.
Add notices for unsafe operations and invalid states (giucam, pq)
 src/wayland-util.h | 224 +++--
 1 file changed, 184 insertions(+), 40 deletions(-)

diff --git a/src/wayland-util.h b/src/wayland-util.h
index cacc122..1ae4f7c 100644
--- a/src/wayland-util.h
+++ b/src/wayland-util.h
@@ -78,73 +78,150 @@ struct wl_interface {

 /** \class wl_list
  *
- * \brief doubly-linked list
+ * \brief Linked list
  *
- * The list head is of "struct wl_list" type, and must be initialized
- * using wl_list_init().  All entries in the list must be of the same
- * type.  The item type must have a "struct wl_list" member. This
- * member will be initialized by wl_list_insert(). There is no need to
- * call wl_list_init() on the individual item. To query if the list is
- * empty in O(1), use wl_list_empty().
+ * On its own, an instance of `struct wl_list` represents the sentinel head of
+ * a circular, linked list, and must be initialized using wl_list_init().
+ * When empty, the list head's `next` and `prev` members point to the list head
+ * itself, otherwise `next` references the first element in the list, and 
`prev`
+ * refers to the last element in the list.
  *
- * Let's call the list reference "struct wl_list foo_list", the item type as
- * "item_t", and the item member as "struct wl_list link".
+ * Use the `struct wl_list` type to represent both the list head and the links
+ * between elements within the list. Use wl_list_empty() to determine if the
+ * list is empty in O(1).
+ *
+ * All elements in the list must be of the same type. The element type must 
have
+ * a `struct wl_list` member, often named `link` by convention. Prior to
+ * insertion, there is no need to initialize an element's `link` - invoking
+ * wl_list_init() on an individual list element's `struct wl_list` member is
+ * unnecessary if the very next operation is wl_list_insert(). However, a
+ * common idiom is to initialize an element's `link` prior to removal - ensure
+ * safety by invoking wl_list_init() before wl_list_remove().
+ *
+ * Consider a list reference `struct wl_list foo_list`, an element type as
+ * `struct element`, and an element's link member as `struct wl_list link`.
+ *
+ * The following code initializes a list and adds three elements to it.
  *
- * The following code will initialize a list:
  * \code
  * struct wl_list foo_list;
  *
- * struct item_t {
- * int foo;
- * struct wl_list link;
+ * struct element {
+ * int foo;
+ * struct wl_list link;
  * };
- * struct item_t item1, item2, item3;
+ * struct element e1, e2, e3;
  *
  * wl_list_init(_list);
- * wl_list_insert(_list, ); // Pushes item1 at the head
- * wl_list_insert(_list, ); // Pushes item2 at the head
- * wl_list_insert(, );   // Pushes item3 after item2
+ * wl_list_insert(_list, );   // e1 is the first element
+ * wl_list_insert(_list, );   // e2 is now the first element
+ * wl_list_insert(, ); // insert e3 after e2
  * \endcode
  *
- * The list now looks like [item2, item3, item1]
+ * The list now looks like [e2, e3, e1].
+ *
+ * The `wl_list` API provides some iterator macros. For example, to iterate
+ * a list in ascending order:
  *
- * Iterate the list in ascending order:
  * \code
- * item_t *item;
- * wl_list_for_each(item, foo_list, link) {
- * Do_something_with_item(item);
+ * struct element *e;
+ * wl_list_for_each(e, foo_list, link) {
+ * do_something_with_element(e);
  * }
  * \endcode
+ *
+ * See the documentation of each iterator for details.
+ * \sa 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/list.h
  */
 struct wl_list {
struct wl_list *prev;
struct wl_list *next;
 };

+/**
+ * Initializes the list.
+ *
+ * \param list List to initialize
+ *
+ * \memberof wl_list
+ */
 void
 wl_list_init(struct wl_list *list);

+/**
+ * Inserts an element into the list, after the element represented by \p list.
+ * When \p list is a reference to the list itself (the head), set the 
containing
+ * struct of \p elm as the first element 

Re: [PATCH] server: Fix crash when accessing client which is already freed

[Giulio Camuffo]

Hi,

could you write how to trigger the crash, also in the commit message
maybe? Besides that, i have a comment inline below.

2016-09-21 9:08 GMT+02:00 Hyunkook Khang :
> While processing pending data, client could be destroyed in the middle of
> the process. (e.g. by invoking wl_display_flush_clients()).
> In this case, client will be freed, but we are still in the processing data
> of the client, so it could cause a crash.
>
> To avoid this, instead of destroying the client directly,
> just set the error here and destroy the client where it needs to be.
>
> Signed-off-by: Hyunkook Khang 
> ---
>  src/wayland-server.c |8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 9d7d9c1..89d0bac 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1103,10 +1103,16 @@ wl_display_terminate(struct wl_display *display)
>  WL_EXPORT void
>  wl_display_run(struct wl_display *display)
>  {
> +   struct wl_client *client, *next;
> +
> display->run = 1;
>
> while (display->run) {
> wl_display_flush_clients(display);
> +   wl_list_for_each_safe(client, next, >client_list, 
> link) {
> +   if (client->error)
> +   wl_client_destroy(client);
> +   }

wl_display_flush_clients() may be called manually by the user too,
without using wl_display_run(). So now all the users would need to do
the loop to destroy the clients, or else they would leak. Instead, you
should do the loop at the end of wl_display_flush_clients().


Thanks,
Giulio

> wl_event_loop_dispatch(display->loop, -1);
> }
>  }
> @@ -1124,7 +1130,7 @@ wl_display_flush_clients(struct wl_display *display)
>   WL_EVENT_WRITABLE |
>   WL_EVENT_READABLE);
> } else if (ret < 0) {
> -   wl_client_destroy(client);
> +   client->error = 1;
> }
> }
>  }
> --
> 1.7.9.5
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel