[PATCH v2 weston 1/6] xwm: fix render format look up

[Uli Schlachter]

On 16.07.2012 16:32, Tiago Vignatti wrote:
> we were using wrong iterator for xcb_render_pictforminfo_t type, the
> formats_reply->length; valgrind was shouting it loudly. Another issue this
> patch addresses is that now find_depth returns the first util and valid format
> that matches the desired depth; it doesn't continue through the loop until the
> end.
>
> This reverts part of commit 5ea11b69.
>
> Signed-off-by: Tiago Vignatti <tiago.vignatti at intel.com>
> ---
>   src/xwayland/window-manager.c |   49 +++++++++++++++++++++++++++--------------
>   1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/src/xwayland/window-manager.c b/src/xwayland/window-manager.c
> index 6e032ea..b922e9b 100644
> --- a/src/xwayland/window-manager.c
> +++ b/src/xwayland/window-manager.c
> @@ -963,6 +963,37 @@ weston_wm_handle_event(int fd, uint32_t mask, void *data)
>   	return count;
>   }
>
> +static xcb_render_pictforminfo_t *
> +find_depth (xcb_connection_t *c, int depth)
> +{
> +	xcb_render_query_pict_formats_reply_t *formats;
> +	xcb_render_query_pict_formats_cookie_t cookie;
> +	xcb_render_pictforminfo_iterator_t i;
> +
> +	cookie = xcb_render_query_pict_formats (c);
> +	xcb_flush (c);

This call to xcb_flush() is unnecessary. XCB will send the request in 
..._reply() if it wasn't sent yet.

> +	formats = xcb_render_query_pict_formats_reply (c, cookie, 0);
> +	if (formats == NULL)
> +		return NULL;
> +
> +	for (i = xcb_render_query_pict_formats_formats_iterator (formats);
> +	     i.rem;
> +	     xcb_render_pictforminfo_next (&i))
> +	{
> +		if (XCB_RENDER_PICT_TYPE_DIRECT != i.data->type)
> +			continue;
> +
> +		if (depth != i.data->depth)
> +			continue;
> +
> +		return i.data;

Doesn't this leak 'formats'?

> +	}
> +
> +	free (formats);
> +	return NULL;
> +}
> +
>   static void
>   wxs_wm_get_resources(struct weston_wm *wm)
>   {
> @@ -1024,15 +1055,10 @@ wxs_wm_get_resources(struct weston_wm *wm)
>   	xcb_xfixes_query_version_reply_t *xfixes_reply;
>   	xcb_intern_atom_cookie_t cookies[ARRAY_LENGTH(atoms)];
>   	xcb_intern_atom_reply_t *reply;
> -	xcb_render_query_pict_formats_reply_t *formats_reply;
> -	xcb_render_query_pict_formats_cookie_t formats_cookie;
> -	xcb_render_pictforminfo_t *formats;
>   	uint32_t i;
>
>   	xcb_prefetch_extension_data (wm->conn, &xcb_xfixes_id);
>
> -	formats_cookie = xcb_render_query_pict_formats(wm->conn);
> -
>   	for (i = 0; i < ARRAY_LENGTH(atoms); i++)
>   		cookies[i] = xcb_intern_atom (wm->conn, 0,
>   					      strlen(atoms[i].name),
> @@ -1059,18 +1085,7 @@ wxs_wm_get_resources(struct weston_wm *wm)
>
>   	free(xfixes_reply);
>
> -	formats_reply = xcb_render_query_pict_formats_reply(wm->conn,
> -							    formats_cookie, 0);
> -	if (formats_reply == NULL)
> -		return;
> -
> -	formats = xcb_render_query_pict_formats_formats(formats_reply);
> -	for (i = 0; i < formats_reply->length; i++)
> -		if (formats[i].type == XCB_RENDER_PICT_TYPE_DIRECT &&
> -		    formats[i].depth == 24)
> -			wm->render_format = formats[i];
> -
> -	free(formats_reply);
> +	wm->render_format = *(find_depth(wm->conn, 24));

This smells like a NULL pointer dereference, some error handling would be nice.

>   }
>
>   static void

Also (this is not specific to this patch, the old code had this, too), what 
happens when the X11 server doesn't have the RENDER extension? The code doesn't 
seem to check for its existence (nor does it check its version). So I guess 
right now libxcb would go into an error state without any kind of remotely 
helpful error message.

Cheers,
Uli