[PATCH] server: Add get/set user data for wl_client

[Pekka Paalanen]

Hi,

I think we should think this a little more.

There is no absolute requirement to add a user_data member to
wl_client, because you can use a destroy listener to look up your user
data struct. When you attach user data, you also want to always be
notified about wl_client destruction, which means you always have a
destroy listener.

The downside of using a destroy listener to look up your user data
struct is that it is an O(number of destroy listeners) operation rather
than O(1), but usually that number should be 1 or 2 I think, your user
data destructor included.

So the first question is, is this really worthwhile?

For comparison to another related patch, adding the client created
signal is obviously worthwhile, since there is no other sure way to be
notified about new wl_clients.

Then let's assume user_data for wl_client is worthwhile. The closest
example of a server-side structure with user data is wl_resource.
wl_resource also has an explicit wl_resource_destroy_func_t member, and
the rationale is clear: if you add user data, you want to be able to
tear down the user data at the right time, so you always need a way to
be notified about destruction.

The second question is, should also wl_client have an explicit user
data destructor function? If yes, you have to add API for it.

The third question is: do all wl_client objects have the same single
owner?

The owner problem is particularly visible in the client side with
wl_proxy, which also has a single user data member of type void
pointer. This creates two problems:

- If wl_proxy should have more than one user data attached, only one of
  them can use the user data member and the others must use the
  destructor trick anyway - oops, wl_proxy does not have a destroy
  signal, but at least wl_client does.

- It is not always obvious who owns the user data. If there are more
  than one component that assume they own the wl_proxy and its user
  data, then passing the wl_proxy to another component that also
  assumes the same causes the other component to access not the data it
  thinks it gets.

The latter problem is especially dangerous when an application is
sharing the same connection with multiple toolkits. Both toolkits may
create wl_surface objects and naturally put their own pointer in the
wl_proxy user data field. They also listen for input events, and input
events carry a reference to a wl_surface. Therefore, a toolkit may
receive a wl_proxy with the other toolkit's user data, and crash or
misbehave due to assuming it is its own.

In the wl_client case, I think the owner issue should be negligible,
there is always one obvious owner for the wl_client and its user data -
or that is what I assume, as I haven't read any other compositor code
than Weston's.

In summary, while there is technically nothing wrong with this idea per
se, I do wonder if it is a net win.

I believe that if there is a dedicated user data slot, there should
also be a dedicated destructor function slot for it.

What do others think? I could go either way.


On Wed, 27 Jan 2016 19:34:56 +0900
Sung-Jin Park <sj76.park at samsung.com> wrote:

> Signed-off-by: Sung-Jin Park <sj76.park at samsung.com>
> ---
>  src/wayland-server-core.h |  6 ++++++
>  src/wayland-server.c      | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index e8e1e9c..6990423 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -186,6 +186,12 @@ int
>  wl_client_get_fd(struct wl_client *client);
>  
>  void
> +wl_client_set_user_data(struct wl_client *client, void *data);
> +
> +void *
> +wl_client_get_user_data(struct wl_client *client);
> +
> +void
>  wl_client_add_destroy_listener(struct wl_client *client,
>  			       struct wl_listener *listener);
>  
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 6654cd7..e7a6eae 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -81,6 +81,7 @@ struct wl_client {
>  	struct wl_signal destroy_signal;
>  	struct ucred ucred;
>  	int error;
> +	void *data;

Please name this user_data.

>  };
>  
>  struct wl_display {
> @@ -526,6 +527,18 @@ wl_client_get_fd(struct wl_client *client)
>  	return wl_connection_get_fd(client->connection);
>  }
>  
> +WL_EXPORT void
> +wl_client_set_user_data(struct wl_client *client, void *data)
> +{
> +	client->data = data;
> +}
> +
> +WL_EXPORT void *
> +wl_client_get_user_data(struct wl_client *client)
> +{
> +	return client->data;
> +}

Documentation is missing.

> +
>  /** Look up an object in the client name space
>   *
>   * \param client The client object

Thanks
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160211/01b01649/attachment.sig>