Hi Olivier, I thought you'd say that. :) Ok, I get it - I did think of those things as well although coming to a different conclusion. However, just consider this...
> On Aug 11, 2016, at 11:32 PM, Olivier Fourdan <ofour...@redhat.com> wrote: > > 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. I agree with your reasons above, but this is a setter associated with a struct member, and the struct member is called global_filter. As such, it seems congruent to call the setter set_global_filter. (Or maybe rename the struct member to filter_global? Perhaps that would bring the naming inline with the reasons that you stated above, plus it matches the setter name.) Respectfully, yong > >>> + 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, >>> ®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, >>> -- > > Meanwhile. I'll send an updated patch with the change I agree with. > > Cheers, > Olivier. > _______________________________________________ > 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