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

[Emil Velikov]

On Friday, 27 May 2016, Tapani Pälli <tapani.palli at intel.com> wrote:

>
>
> On 05/26/2016 05:16 PM, Emil Velikov wrote:
>
>> 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 ?
>>
>>
> No progress, this patch has been living as is in internal project. The fix
> itself is quite simple, all places with __glXGetCurrentContext should check
> against dummyContext.
>
> Indeed. I believe I mentioned /suggested that ;-)


> This patch introduced its own 'dummyContext' in the unit test since it
> seemed very challenging to compile the test together with files in glx
> folder (results in linking with a *lot* of stuff). I can take a peek again
> what was the issue it replacing all of the checks and reply back to this.
>
> I think there were some concerns
 - not everything going is updated (Ian), looks fine now but will need to
double check
 - piglits (Ian)
 - some mesa glx tests are crashing/failing (yourself mentioned that iirc).
I take it that things are fine now ?

Thanks
Emil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160527/08c058c5/attachment.html>