[Mesa-dev] [PATCH 3/3] [Bug 38970] [bisected]piglit glx/glx-pixmap-multi failed

[Ian Romanick]

On 09/29/2016 01:55 PM, Anutex wrote:
> I tried to debug this issue with changing the condition to check only bad magic and Error.
> And the test passed.
> 
> Though i am not sure what is the correct behaviour if we are in this condition.
> May be we should make some  other condition if the Hash Table have the bucket data.
> ---
>  src/glx/dri2_glx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
> index af388d9..a1fd9ff 100644
> --- a/src/glx/dri2_glx.c
> +++ b/src/glx/dri2_glx.c
> @@ -411,12 +411,13 @@ dri2CreateDrawable(struct glx_screen *base, XID xDrawable,
>        return NULL;
>     }
>  
> -   if (__glxHashInsert(pdp->dri2Hash, xDrawable, pdraw)) {
> +   if (__glxHashInsert(pdp->dri2Hash, xDrawable, pdraw) == -1) {

I'm not 100% sure the existing code is wrong.  __glxHashInsert returns
-1 for an error, and it returns 1 if the key is already in the hash
table.  In that case we'll leak the memory for the new pdraw, right?
That also seems bad.

It seems like instead the code should look up xDrawable in the hash
table and return the value that's already there.  Maybe.  I haven't
looked at this code in years, so I may be forgetting some subtlety.

>        (*psc->core->destroyDrawable) (pdraw->driDrawable);
>        DRI2DestroyDrawable(psc->base.dpy, xDrawable);
>        free(pdraw);
>        return None;
>     }
> +	
>  

Spurious whitespace change.

>     /*
>      * Make sure server has the same swap interval we do for the new
>