Re: [PATCH weston v2 2/2] libweston-desktop/xdg-shell: Properly handle ack_configure

[Quentin Glidic]

On 7/20/17 5:56 AM, Jonas Ådahl wrote:
On Tue, Jul 18, 2017 at 01:14:49PM +0200, Quentin Glidic wrote:
From: Quentin Glidic <sardemff7+...@sardemff7.net>
Now we keep track of serial->state association and we discard the states
that the client ignored.

Signed-off-by: Quentin Glidic <sardemff7+...@sardemff7.net>
---
  libweston-desktop/xdg-shell-v5.c | 45 +++++++++++++++++++++++-----
  libweston-desktop/xdg-shell-v6.c | 64 +++++++++++++++++++++++++++++++++-------
  2 files changed, 92 insertions(+), 17 deletions(-)

diff --git a/libweston-desktop/xdg-shell-v5.c b/libweston-desktop/xdg-shell-v5.c
index b32b7812..f9f7a5d1 100644
--- a/libweston-desktop/xdg-shell-v5.c
+++ b/libweston-desktop/xdg-shell-v5.c
@@ -46,6 +46,13 @@ struct weston_desktop_xdg_surface_state {
        bool activated;
  };
  
+struct weston_desktop_xdg_surface_configure {
+       struct wl_list link; /* weston_desktop_xdg_surface::configure_list */
+       uint32_t serial;
+       struct weston_desktop_xdg_surface_state state;
+       struct weston_size size;
+};
+
  struct weston_desktop_xdg_surface {
        struct wl_resource *resource;
        struct weston_desktop_surface *surface;
@@ -53,7 +60,7 @@ struct weston_desktop_xdg_surface {
        bool added;
        struct wl_event_source *add_idle;
        struct wl_event_source *configure_idle;
-       uint32_t configure_serial;
+       struct wl_list configure_list; /* 
weston_desktop_xdg_surface_configure::link */
        struct {
                struct weston_desktop_xdg_surface_state state;
                struct weston_size size;
@@ -94,13 +101,26 @@ static void
  weston_desktop_xdg_surface_send_configure(void *data)
  {
        struct weston_desktop_xdg_surface *surface = data;
+       struct weston_desktop_xdg_surface_configure *configure;
        uint32_t *s;
        struct wl_array states;
  
  	surface->configure_idle = NULL;
  
-	surface->configure_serial =
+       configure = zalloc(sizeof(struct weston_desktop_xdg_surface_configure));
+       if ( configure == NULL ) {

No spacing around expression.

Oops, fixed.


+               struct weston_desktop_client *client =
+                       weston_desktop_surface_get_client(surface->surface);
+               struct wl_client *wl_client =
+                       weston_desktop_client_get_client(client);
+               wl_client_post_no_memory(wl_client);
+               return;
+       }
+       wl_list_insert(surface->configure_list.prev, &configure->link);
+       configure->serial =
                
wl_display_next_serial(weston_desktop_get_display(surface->desktop));
+       configure->state = surface->pending.state;
+       configure->size = surface->pending.size;
  
  	wl_array_init(&states);
        if (surface->pending.state.maximized) {
@@ -124,7 +144,7 @@ weston_desktop_xdg_surface_send_configure(void *data)
                                   surface->pending.size.width,
                                   surface->pending.size.height,
                                   &states,
-                                  surface->configure_serial);
+                                  configure->serial);
  
  	wl_array_release(&states);
  };
@@ -443,12 +463,23 @@ weston_desktop_xdg_surface_protocol_ack_configure(struct 
wl_client *wl_client,
                wl_resource_get_user_data(resource);
        struct weston_desktop_xdg_surface *surface =
                weston_desktop_surface_get_implementation_data(dsurface);
-
-       if (surface->configure_serial != serial)
+       struct weston_desktop_xdg_surface_configure *configure, *temp;
+
+       wl_list_for_each_safe(configure, temp, &surface->configure_list, link) {
+               if (configure->serial < serial) {
+                       wl_list_remove(&configure->link);
+                       free(configure);
+               } else if (configure->serial == serial) {
+                       wl_list_remove(&configure->link);
+                       break;
+               }
                return;
+       }
+
+       surface->next.state = configure->state;
+       surface->next.size = configure->size;

Must check that we found the acked configure. Bad clients can send a
bogus ack serial.

  
-	surface->next.state = surface->pending.state;
-       surface->next.size = surface->pending.size;
+       free(configure);
  }
  
  static void
diff --git a/libweston-desktop/xdg-shell-v6.c b/libweston-desktop/xdg-shell-v6.c
index f5e46daa..9094260e 100644
--- a/libweston-desktop/xdg-shell-v6.c
+++ b/libweston-desktop/xdg-shell-v6.c
@@ -69,7 +69,7 @@ struct weston_desktop_xdg_surface {
        struct weston_desktop_surface *desktop_surface;
        bool configured;
        struct wl_event_source *configure_idle;
-       uint32_t configure_serial;
+       struct wl_list configure_list; /* 
weston_desktop_xdg_surface_configure::link */
  
  	bool has_next_geometry;
        struct weston_geometry next_geometry;
@@ -77,6 +77,11 @@ struct weston_desktop_xdg_surface {
        enum weston_desktop_xdg_surface_role role;
  };
  
+struct weston_desktop_xdg_surface_configure {
+       struct wl_list link; /* weston_desktop_xdg_surface::configure_list */
+       uint32_t serial;
+};
+
  struct weston_desktop_xdg_toplevel_state {
        bool maximized;
        bool fullscreen;
@@ -84,6 +89,12 @@ struct weston_desktop_xdg_toplevel_state {
        bool activated;
  };
  
+struct weston_desktop_xdg_toplevel_configure {
+       struct weston_desktop_xdg_surface_configure base;
+       struct weston_desktop_xdg_toplevel_state state;
+       struct weston_size size;
+};
+
  struct weston_desktop_xdg_toplevel {
        struct weston_desktop_xdg_surface base;
  
@@ -115,6 +126,7 @@ struct weston_desktop_xdg_popup {
  };
  
  #define weston_desktop_surface_role_biggest_size (sizeof(struct weston_desktop_xdg_toplevel))
+#define weston_desktop_surface_configure_biggest_size (sizeof(struct 
weston_desktop_xdg_toplevel))
  
  
  static struct weston_geometry
@@ -422,10 +434,11 @@ weston_desktop_xdg_toplevel_protocol_resize(struct 
wl_client *wl_client,
  }
  
  static void
-weston_desktop_xdg_toplevel_ack_configure(struct weston_desktop_xdg_toplevel 
*toplevel)
+weston_desktop_xdg_toplevel_ack_configure(struct weston_desktop_xdg_toplevel 
*toplevel,
+                                         struct 
weston_desktop_xdg_toplevel_configure *configure)
  {
-       toplevel->next.state = toplevel->pending.state;
-       toplevel->next.size = toplevel->pending.size;
+       toplevel->next.state = configure->state;
+       toplevel->next.size = configure->size;
  }
  
  static void
@@ -529,11 +542,15 @@ weston_desktop_xdg_toplevel_protocol_set_minimized(struct 
wl_client *wl_client,
  }
  
  static void
-weston_desktop_xdg_toplevel_send_configure(struct weston_desktop_xdg_toplevel 
*toplevel)
+weston_desktop_xdg_toplevel_send_configure(struct weston_desktop_xdg_toplevel 
*toplevel,
+                                          struct 
weston_desktop_xdg_toplevel_configure *configure)
  {
        uint32_t *s;
        struct wl_array states;
  
+	configure->state = toplevel->pending.state;
+       configure->size = toplevel->pending.size;
+
        wl_array_init(&states);
        if (toplevel->pending.state.maximized) {
                s = wl_array_add(&states, sizeof(uint32_t));
@@ -848,9 +865,21 @@ static void
  weston_desktop_xdg_surface_send_configure(void *user_data)
  {
        struct weston_desktop_xdg_surface *surface = user_data;
+       struct weston_desktop_xdg_surface_configure *configure;
  
  	surface->configure_idle = NULL;
-       surface->configure_serial =
+
+       configure = zalloc(weston_desktop_surface_configure_biggest_size);
+       if ( configure == NULL ) {
+               struct weston_desktop_client *client =
+                       
weston_desktop_surface_get_client(surface->desktop_surface);
+               struct wl_client *wl_client =
+                       weston_desktop_client_get_client(client);
+               wl_client_post_no_memory(wl_client);
+               return;
+       }
+       wl_list_insert(surface->configure_list.prev, &configure->link);
+       configure->serial =
                
wl_display_next_serial(weston_desktop_get_display(surface->desktop));
  
  	switch (surface->role) {
@@ -858,14 +887,15 @@ weston_desktop_xdg_surface_send_configure(void *user_data)
                assert(0 && "not reached");
                break;
        case WESTON_DESKTOP_XDG_SURFACE_ROLE_TOPLEVEL:
-               weston_desktop_xdg_toplevel_send_configure((struct 
weston_desktop_xdg_toplevel *) surface);
+               weston_desktop_xdg_toplevel_send_configure((struct 
weston_desktop_xdg_toplevel *) surface,
+                                                          (struct 
weston_desktop_xdg_toplevel_configure *) configure);
                break;
        case WESTON_DESKTOP_XDG_SURFACE_ROLE_POPUP:
                weston_desktop_xdg_popup_send_configure((struct 
weston_desktop_xdg_popup *) surface);
                break;
        }
  
-	zxdg_surface_v6_send_configure(surface->resource, surface->configure_serial);
+       zxdg_surface_v6_send_configure(surface->resource, configure->serial);
  }
  
  static bool
@@ -1060,12 +1090,21 @@ 
weston_desktop_xdg_surface_protocol_ack_configure(struct wl_client *wl_client,
                wl_resource_get_user_data(resource);
        struct weston_desktop_xdg_surface *surface =
                weston_desktop_surface_get_implementation_data(dsurface);
+       struct weston_desktop_xdg_surface_configure *configure, *temp;
  
  	if (!weston_desktop_xdg_surface_check_role(surface))
                return;
  
-	if (surface->configure_serial != serial)
+       wl_list_for_each_safe(configure, temp, &surface->configure_list, link) {
+               if (configure->serial < serial) {
+                       wl_list_remove(&configure->link);
+                       free(configure);
+               } else if (configure->serial == serial) {
+                       wl_list_remove(&configure->link);
+                       break;
+               }
                return;
+       }

Same here as above about checking we found a corresponding configure.

True, that would even crash if the serial is wrong.


I guess for both those cases it's safe to say raising a client error is
reasonable, as a functioning client should never send bogus
ack_configures.

I am not sure which error to send for both shells so I went with 
invalid_surface_state for v6 and defunct_surfaces for v5.

New patch coming.

Cheers,

Jonas


  
  	surface->configured = true;
  
@@ -1074,11 +1113,14 @@ weston_desktop_xdg_surface_protocol_ack_configure(struct wl_client *wl_client,
                assert(0 && "not reached");
                break;
        case WESTON_DESKTOP_XDG_SURFACE_ROLE_TOPLEVEL:
-               weston_desktop_xdg_toplevel_ack_configure((struct 
weston_desktop_xdg_toplevel *) surface);
+               weston_desktop_xdg_toplevel_ack_configure((struct 
weston_desktop_xdg_toplevel *) surface,
+                                                         (struct 
weston_desktop_xdg_toplevel_configure *) configure);
                break;
        case WESTON_DESKTOP_XDG_SURFACE_ROLE_POPUP:
                break;
        }
+
+       free(configure);
  }
  
  static void
@@ -1290,6 +1332,8 @@ weston_desktop_xdg_shell_protocol_get_xdg_surface(struct 
wl_client *wl_client,
                                       "xdg_surface must not have a buffer at 
creation");
                return;
        }
+
+       wl_list_init(&surface->configure_list);
  }
  
  static void
--
2.13.3

_______________________________________________
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



--

Quentin “Sardem FF7” Glidic
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel