Hi Olivier, On Aug 8, 2016, at 1:10 AM, Olivier Fourdan <ofour...@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 <ofour...@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@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