[PATCH weston v2 10/24] xwm: clarify props[] in weston_wm_window_read_properties()

[Quentin Glidic]

On 21/12/2016 15:40, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> The props array contained offsets to struct members. It is convenient
> for writing static const arrays as you only store a constant offset and
> compute the pointer later. However, the array was not static to begin
> with, the atoms are not build time constants. We can as well just store
> the pointer directly in the array.
> 
> Entries that did not use the offset had bogus offsets, producing
> pointers to arbitrary fields. They are changed to have a NULL pointer.
> If the code unintentionally used the pointer, it will now explode rather
> than corrupt memory.
> 
> Also explain the use of the #defined constants and #undef them when they
> get out of scope. This clearly documents that they are just a convenient
> hack to avoid lots of special cases in the function.
> 
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Much cleaner:
Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>

Thanks,


> ---
>   xwayland/window-manager.c | 38 +++++++++++++++++++++++---------------
>   1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index 02a44a7..c4220ab 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -443,7 +443,10 @@ read_and_dump_property(struct weston_wm *wm,
>   	free(reply);
>   }
>   
> -/* We reuse some predefined, but otherwise useles atoms */
> +/* We reuse some predefined, but otherwise useles atoms
> + * as local type placeholders that never touch the X11 server,
> + * to make weston_wm_window_read_properties() less exceptional.
> + */
>   #define TYPE_WM_PROTOCOLS	XCB_ATOM_CUT_BUFFER0
>   #define TYPE_MOTIF_WM_HINTS	XCB_ATOM_CUT_BUFFER1
>   #define TYPE_NET_WM_STATE	XCB_ATOM_CUT_BUFFER2
> @@ -456,23 +459,23 @@ weston_wm_window_read_properties(struct weston_wm_window *window)
>   	const struct weston_desktop_xwayland_interface *xwayland_interface =
>   		wm->server->compositor->xwayland_interface;
>   
> -#define F(field) offsetof(struct weston_wm_window, field)
> +#define F(field) (&window->field)
>   	const struct {
>   		xcb_atom_t atom;
>   		xcb_atom_t type;
> -		int offset;
> +		void *ptr;
>   	} props[] = {
> -		{ XCB_ATOM_WM_CLASS, XCB_ATOM_STRING, F(class) },
> -		{ XCB_ATOM_WM_NAME, XCB_ATOM_STRING, F(name) },
> -		{ XCB_ATOM_WM_TRANSIENT_FOR, XCB_ATOM_WINDOW, F(transient_for) },
> -		{ wm->atom.wm_protocols, TYPE_WM_PROTOCOLS, F(protocols) },
> -		{ wm->atom.wm_normal_hints, TYPE_WM_NORMAL_HINTS, F(protocols) },
> -		{ wm->atom.net_wm_state, TYPE_NET_WM_STATE },
> -		{ wm->atom.net_wm_window_type, XCB_ATOM_ATOM, F(type) },
> -		{ wm->atom.net_wm_name, XCB_ATOM_STRING, F(name) },
> -		{ wm->atom.net_wm_pid, XCB_ATOM_CARDINAL, F(pid) },
> -		{ wm->atom.motif_wm_hints, TYPE_MOTIF_WM_HINTS, 0 },
> -		{ wm->atom.wm_client_machine, XCB_ATOM_WM_CLIENT_MACHINE, F(machine) },
> +		{ XCB_ATOM_WM_CLASS,           XCB_ATOM_STRING,            F(class) },
> +		{ XCB_ATOM_WM_NAME,            XCB_ATOM_STRING,            F(name) },
> +		{ XCB_ATOM_WM_TRANSIENT_FOR,   XCB_ATOM_WINDOW,            F(transient_for) },
> +		{ wm->atom.wm_protocols,       TYPE_WM_PROTOCOLS,          NULL },
> +		{ wm->atom.wm_normal_hints,    TYPE_WM_NORMAL_HINTS,       NULL },
> +		{ wm->atom.net_wm_state,       TYPE_NET_WM_STATE,          NULL },
> +		{ wm->atom.net_wm_window_type, XCB_ATOM_ATOM,              F(type) },
> +		{ wm->atom.net_wm_name,        XCB_ATOM_STRING,            F(name) },
> +		{ wm->atom.net_wm_pid,         XCB_ATOM_CARDINAL,          F(pid) },
> +		{ wm->atom.motif_wm_hints,     TYPE_MOTIF_WM_HINTS,        NULL },
> +		{ wm->atom.wm_client_machine,  XCB_ATOM_WM_CLIENT_MACHINE, F(machine) },
>   	};
>   #undef F
>   
> @@ -511,7 +514,7 @@ weston_wm_window_read_properties(struct weston_wm_window *window)
>   			continue;
>   		}
>   
> -		p = ((char *) window + props[i].offset);
> +		p = props[i].ptr;
>   
>   		switch (props[i].type) {
>   		case XCB_ATOM_WM_CLIENT_MACHINE:
> @@ -605,6 +608,11 @@ weston_wm_window_read_properties(struct weston_wm_window *window)
>   		xwayland_interface->set_pid(window->shsurf, window->pid);
>   }
>   
> +#undef TYPE_WM_PROTOCOLS
> +#undef TYPE_MOTIF_WM_HINTS
> +#undef TYPE_NET_WM_STATE
> +#undef TYPE_WM_NORMAL_HINTS
> +
>   static void
>   weston_wm_window_get_frame_size(struct weston_wm_window *window,
>   				int *width, int *height)
> 


-- 

Quentin “Sardem FF7” Glidic