[PATCH 1/3 v3] wayland-server: Add API to control globals visibility
Yong Bakos
junk at humanoriented.com
Thu Aug 11 22:42:50 UTC 2016
Hi Olivier,
On Aug 8, 2016, at 1:10 AM, Olivier Fourdan <ofourdan at redhat.com> wrote:
>
> Add a new API to let compositor decide whether or not a wl_global
> should be advertised to the clients via wl_registry_bind() or
> display_get_registry()
>
> By using its own filter, the compositor can decide which wl_global would
> be listed to clients.
>
> Compositors can use this mechanism to hide their own private interfaces
> that regular clients should not use.
>
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
My comments are inline below, should be straightforward. Also note
that there will need to be a rebase now (giucam's recent patches).
> ---
> v2: Follow-up on Jonas' comments on v1:
> Add global's data as user data in callback filter function
> Pass wl_global instead of wl_interface in callback filter function
> Add wl_global_get_interface() to retrieve the wl_interface from the
> given wl_global
> Post an error if a client tries to bind a global which is filtered
> out.
> v3: Follow-up on Jonas' comments on v2:
> Separate from the other wl_global and test additions
> Add its own user data to the global filter
> Rephrase the API doc
> Other few small changes
>
> src/wayland-server-core.h | 10 ++++++++
> src/wayland-server.c | 62 +++++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index ad1292f..b70d85f 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -28,6 +28,7 @@
>
> #include <sys/types.h>
> #include <stdint.h>
> +#include <stdbool.h>
> #include "wayland-util.h"
> #include "wayland-version.h"
>
> @@ -164,6 +165,15 @@ wl_global_create(struct wl_display *display,
> void
> wl_global_destroy(struct wl_global *global);
>
> +typedef bool (*wl_display_filter_global_func_t)(const struct wl_client *client,
After reading this patch a few times, and writing some sample use cases,
I really think this type should be named wl_display_global_filter_func_t,
meaning "this is a global filter function."
Second, perhaps a short comment is necessary here, before the typedef, that
states something like:
"A filter function enables the server to decide which globals to
advertise to clients. This function should return true if..."
Furthermore,
> + const struct wl_global *global,
> + void *data);
> +
> +void
> +wl_display_set_filter_global(struct wl_display *display,
This should be set_global_filter, matching the name of the struct member...
> + wl_display_filter_global_func_t filter,
> + void *data);
> +
> struct wl_client *
> wl_client_create(struct wl_display *display, int fd);
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 19aa2e8..480af23 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -98,6 +98,9 @@ struct wl_display {
> struct wl_signal destroy_signal;
>
> struct wl_array additional_shm_formats;
> +
> + wl_display_filter_global_func_t global_filter;
... Here, where again the type is better described as
wl_display_global_filter_func_t.
> + void *global_filter_data;
> };
>
> struct wl_global {
> @@ -714,6 +717,16 @@ wl_client_destroy(struct wl_client *client)
> free(client);
> }
>
I would add a short comment here for the filter_global function,
that at least explains when it returns true. /Better yet/, if you
named this function, say, `is_visible`, `is_advertised`, `is_listed`,
or perhaps even
wl_global_is_visible(const struct wl_global *global,
const struct wl_client *client)
Then its usage within registry_bind and display_get_registry
becomes very clear.
Regards,
yong
> +static bool
> +filter_global(const struct wl_client *client,
> + const struct wl_global *global)
> +{
> + struct wl_display *display = client->display;
> +
> + return (display->global_filter == NULL ||
> + display->global_filter(client, global, display->global_filter_data));
> +}
> +
> static void
> registry_bind(struct wl_client *client,
> struct wl_resource *resource, uint32_t name,
> @@ -740,6 +753,10 @@ registry_bind(struct wl_client *client,
> WL_DISPLAY_ERROR_INVALID_OBJECT,
> "invalid version for global %s (%d): have %d, wanted %d",
> interface, name, global->version, version);
> + else if (!filter_global(client, global))
> + wl_resource_post_error(resource,
> + WL_DISPLAY_ERROR_INVALID_OBJECT,
> + "invalid global %s (%d)", interface, name);
> else
> global->bind(client, global->data, version, id);
> }
> @@ -795,11 +812,12 @@ display_get_registry(struct wl_client *client,
> ®istry_resource->link);
>
> wl_list_for_each(global, &display->global_list, link)
> - wl_resource_post_event(registry_resource,
> - WL_REGISTRY_GLOBAL,
> - global->name,
> - global->interface->name,
> - global->version);
> + if (filter_global(client, global))
> + wl_resource_post_event(registry_resource,
> + WL_REGISTRY_GLOBAL,
> + global->name,
> + global->interface->name,
> + global->version);
> }
>
> static const struct wl_display_interface display_interface = {
> @@ -868,6 +886,9 @@ wl_display_create(void)
> display->id = 1;
> display->serial = 0;
>
> + display->global_filter = NULL;
> + display->global_filter_data = NULL;
> +
> wl_array_init(&display->additional_shm_formats);
>
> return display;
> @@ -940,6 +961,37 @@ wl_display_destroy(struct wl_display *display)
> free(display);
> }
>
> +/** Set a filter function for global objects
> + *
> + * \param display The Wayland display object.
> + * \param filter The global filter funtion.
> + * \param data User data to be associated with the global filter.
> + * \return None.
> + *
> + * Set a filter for the wl_display to advertise or hide global objects
> + * to clients.
> + * The set filter will be used during wl_global advertisment to
> + * determine whether a global object should be advertised to a
> + * given client, and during wl_global binding to determine whether
> + * a given client should be allowed to bind to a global.
> + *
> + * Clients that try to bind to a global that was filtered out will
> + * have an error raised.
> + *
> + * Setting the filter NULL will result in all globals being
> + * advertised to all clients. The default is no filter.
> + *
> + * \memberof wl_display
> + */
> +WL_EXPORT void
> +wl_display_set_filter_global(struct wl_display *display,
> + wl_display_filter_global_func_t filter,
> + void *data)
> +{
> + display->global_filter = filter;
> + display->global_filter_data = data;
> +}
> +
> WL_EXPORT struct wl_global *
> wl_global_create(struct wl_display *display,
> const struct wl_interface *interface, int version,
> --
> 2.7.4
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list