[PATCH weston] xwm: Add and use helper function for looking up windows in the hash table

[Pekka Paalanen]

On Wed,  8 Apr 2015 13:35:44 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> This lets us verify that all callers are actually testing for a
> successful hash lookup at compile time.
> 
> All current users of hash_table_lookup are converted to the new
> wm_lookup_window() and the appropriate success check is added.
> This fixes any call sites that used to assume a successful return
> and dereference a NULL pointer.
> 
> This closes http://bugs.freedesktop.org/show_bug.cgi?id=83994
> The xwayland test has been failing because weston crashes due to
> a hash lookup failure and a subsequent dereference of the returned
> NULL pointer.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
> 
> This is a rewrite of two previous patches, differences are:
> 
> Use a helper function instead of changing hash_table_lookup
> 
> combine the two patches into one
> 
> try to limit the use of temporary variables to places where if
> statements become ugly (due to 80 col constraints).
> 
>  xwayland/window-manager.c | 91 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 70 insertions(+), 21 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index 83ebfae..966962e 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -195,6 +195,15 @@ wm_log_continue(const char *fmt, ...)
>  #endif
>  }
>  
> +static bool __attribute__ ((warn_unused_result))
> +wm_lookup_window(struct hash_table *ht, uint32_t hash,
> +		      struct weston_wm_window **window)

Hi,

you could make the call sites even shorter if the argument here was a
struct weston_wm *wm instead of struct hash_table *ht. ;-)

Also the uint32_t hash... isn't that always a Window? XID? Would it
work to have that type as Window or whatever it actually always is?
Going to a more explicit type would be a documentation improvement.

> +{
> +	*window = hash_table_lookup(ht, hash);
> +	if (*window)
> +		return true;
> +	return false;
> +}

The patch looks good to me, so
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
in any case.


Thanks,
pq