[PATCH 3/4] xwayland: Keep a list of wayland globals

[Dima Ryazanov]

On Thu, May 14, 2015 at 12:57 AM, Marek Chalupa <mchqwerty at gmail.com> wrote:

> On Tue, May 12, 2015 at 7:21 PM, Dima Ryazanov <dima at gmail.com> wrote:
>
>> The logic is pretty much copied from weston's clients/window.c.
>>
>> Signed-off-by: Dima Ryazanov <dima at gmail.com>
>> ---
>>  hw/xwayland/xwayland.c | 25 ++++++++++++++++++++++++-
>>  hw/xwayland/xwayland.h |  8 ++++++++
>>  2 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
>> index 7e8d667..e99fbac 100644
>> --- a/hw/xwayland/xwayland.c
>> +++ b/hw/xwayland/xwayland.c
>> @@ -383,6 +383,17 @@ registry_global(void *data, struct wl_registry
>> *registry, uint32_t id,
>>                  const char *interface, uint32_t version)
>>  {
>>      struct xwl_screen *xwl_screen = data;
>> +    struct xwl_global *xwl_global;
>> +
>> +    xwl_global = calloc(sizeof *xwl_global, 1);
>> +    if (xwl_global == NULL) {
>> +        ErrorF("registry_global ENOMEM\n");
>> +        return;
>> +    }
>> +    xwl_global->name = id;
>> +    xwl_global->interface = strdup(interface);
>>
>
> You should probably check if the strdup succeeds here. In the following
> patch you do
>   if (strcmp(xwl_global->interface, "wl_output") == 0)
> and if the strdup returns NULL, then this contition will be false even
> thoug it should be true.
>

Oh, good catch.


>
>
>> +    xwl_global->version = version;
>> +    xorg_list_add(&xwl_global->link, &xwl_screen->global_list);
>>
>>      if (strcmp(interface, "wl_compositor") == 0) {
>>          xwl_screen->compositor =
>> @@ -410,7 +421,18 @@ registry_global(void *data, struct wl_registry
>> *registry, uint32_t id,
>>  static void
>>  global_remove(void *data, struct wl_registry *registry, uint32_t name)
>>  {
>> -    /* Nothing to do here, wl_compositor and wl_shm should not be
>> removed */
>> +    struct xwl_screen *xwl_screen = data;
>> +    struct xwl_global *xwl_global, *next_xwl_global;
>> +
>> +    xorg_list_for_each_entry_safe(xwl_global, next_xwl_global,
>> +                                  &xwl_screen->global_list, link) {
>> +        if (xwl_global->name != name)
>> +            continue;
>> +
>> +        xorg_list_del(&xwl_global->link);
>> +        free(xwl_global->interface);
>> +        free(xwl_global);
>>
>
> Here a break would be handy, so that we won't iterate over the rest of
> globals after we found the one we need.
>

Sure.


> +    }
>>  }
>>
>>  static const struct wl_registry_listener registry_listener = {
>> @@ -562,6 +584,7 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char
>> **argv)
>>              listen_on_fds(xwl_screen);
>>      }
>>
>> +    xorg_list_init(&xwl_screen->global_list);
>>      xorg_list_init(&xwl_screen->output_list);
>>      xorg_list_init(&xwl_screen->seat_list);
>>      xorg_list_init(&xwl_screen->damage_window_list);
>> diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
>> index cfb343d..2ba5312 100644
>> --- a/hw/xwayland/xwayland.h
>> +++ b/hw/xwayland/xwayland.h
>> @@ -64,6 +64,7 @@ struct xwl_screen {
>>      UnrealizeWindowProcPtr UnrealizeWindow;
>>      XYToWindowProcPtr XYToWindow;
>>
>> +    struct xorg_list global_list;
>>      struct xorg_list output_list;
>>      struct xorg_list seat_list;
>>      struct xorg_list damage_window_list;
>> @@ -95,6 +96,13 @@ struct xwl_screen {
>>      struct glamor_context *glamor_ctx;
>>  };
>>
>> +struct xwl_global {
>> +    uint32_t name;
>> +    char *interface;
>> +    uint32_t version;
>> +    struct xorg_list link;
>> +};
>> +
>>  struct xwl_window {
>>      struct xwl_screen *xwl_screen;
>>      struct wl_surface *surface;
>> --
>> 2.4.0
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
> Isn't storing all globals redundant for this purpose? Couldn't we add the
> name (id) into struct xwl_output and then
> iterate just over the output_list?
>

Correct - if we only care about xwl_output, then the globals list is
unnecessary. I only did it that way because that's how weston does it, and
it feels more generic - it makes it easy to handle different types of
globals in the future. That said, it offers no performance advantages since
it's just iterating over the whole list. A hash table would make more sense.

Anyways, I'll remove it for now.


>
> Thanks,
> Marek
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150514/137449b5/attachment-0001.html>