[Mesa-dev] [Mesa-stable] [PATCH 4/4] glx: fix error code when there is no context bound

[Emil Velikov]

Hi all,

On 29 February 2016 at 07:14, Tapani Pälli <tapani.palli at intel.com> wrote:
>
> On 02/22/2016 10:16 PM, Ian Romanick wrote:
>>
>> There are 17 total occurrences of
>>
>>      grep -r '[(]!gc[)]' src/glx/
>>
>> and
>>
>>      grep -r 'gc[[:space:]]*==[[:space:]]*NULL' src/glx/
>>
>> None of these check for dummyContext.  This is all very suspicious.
>> Looking at the implementation(s) of __glXGetCurrentContext, I don't
>> think it can ever return NULL.  Look in src/glx/glxcurrent.c.  It's
>> possible that __glXGetCurrentContext used to be able to return NULL, but
>> I find it unlikely.
>>
>> My guess is that all (or nearly all) of the !gc or gc == NULL checks are
>> wrong.  A bunch of them probably "just work" because they end up sending
>> protocol requests to the server, and the server sends back an error.
>
>
> I spent some time with this and it looks like some of these are correct as
> create_context (or indirect_create_context) can return NULL and also pointer
> given by client may be NULL (and can't be dummyContext). The places with
> explicit __glXGetCurrentContext call (9 of these) and a NULL check are
> incorrect. I can add these to the patch.
>
>> At the very least, I think these gc == NULL checks should be replaced by
>> asserts.  If the unit tests call these functions with
>> __glXGetCurrentContext returning NULL, the unit tests should be fixed to
>> return &dummyContext instead.
>
>
> Should it be then 'own dummyContext' implemented by fake_glx_screen.cpp
> something along lines in this patch and not trying to link with
> glxcurrent.c?
>
>> I'd really like to see analysis of the other NULL checks and either have
>> justifications for no change or have changes.  I'd also really like to
>> see piglit tests that could hit some of these.
>
>
> It looks like glx-test is testing return value of __glXGetCurrentContext
> currently (which is why it breaks), wouldn't fixing glx-test be sufficient?
>
>
Any news on the status of this patch ? The Suse guys did bring some
fixes recently (check __glXGetCurrentContext() vs dummyContext as
opposed to NULL), although I think we still want something like the
proposed here. Correct ?

Thanks
Emil