[PATCH 1/3 v3] wayland-server: Add API to control globals visibility
Yong Bakos
junk at humanoriented.com
Fri Aug 12 17:23:36 UTC 2016
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 <ofourdan at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list