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 <[email protected]> wrote: > Signed-off-by: Sung-Jin Park <[email protected]> > --- > 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
pgpjFvMZWzOwz.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
