[PATCH 1/3 v3] wayland-server: Add API to control globals visibility

[Olivier Fourdan]

Hi Yong,

Thanks for your follow-up.

I don;t necessarily agree with your all of comments though, see below.

> > @@ -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."

Err, nope, I disagree, it's not what is meant, precisely.

Using something like "global_filter" makes it sound like "global" is an adjective for the filter, i.e. the filter is global, which is not the meaning of global here, global here is a noun for "wl_global" (but "wl_display_filter_wl_global_func_t" would sound awkward).

I intentionally named it "filter_global" to avoid that confusion.

> 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..."

Yeap, I agree.

> > +						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...

Nope, for the same reasons as above.

> > +			     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.

Nope, for the same reasons as above.

> > +	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.

Yeap, makes sense, agreed.
 
> > +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,
> > 		       &registry_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,
> > --

Meanwhile. I'll send an updated patch with the change I agree with.

Cheers,
Olivier.