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

[Tapani Pälli]

On 02/12/2016 02:54 AM, Emil Velikov wrote:
> On 11 February 2016 at 12:04, Tapani Pälli <tapani.palli at intel.com> wrote:
>> From: Bernard Kilarski <bernard.r.kilarski at intel.com>
>>
>> Signed-off-by: Bernard Kilarski <bernard.r.kilarski at intel.com>
>> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> Cc: "11.0 11.1" <mesa-stable at lists.freedesktop.org
>> ---
>>   src/glx/glxcmds.c                         | 2 +-
>>   src/glx/query_renderer.c                  | 4 ++--
>>   src/glx/tests/query_renderer_unittest.cpp | 4 ++++
>>   3 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
>> index 93e8db5..4db67ec 100644
>> --- a/src/glx/glxcmds.c
>> +++ b/src/glx/glxcmds.c
>> @@ -1727,7 +1727,7 @@ __glXSwapIntervalSGI(int interval)
>>      CARD32 *interval_ptr;
>>      CARD8 opcode;
>>
>> -   if (gc == NULL) {
>> +   if (gc == NULL || gc == &dummyContext) {
>>         return GLX_BAD_CONTEXT;
>>      }
>>
>> diff --git a/src/glx/query_renderer.c b/src/glx/query_renderer.c
>> index 9108ec2..d49b8fe 100644
>> --- a/src/glx/query_renderer.c
>> +++ b/src/glx/query_renderer.c
>> @@ -106,7 +106,7 @@ glXQueryCurrentRendererIntegerMESA(int attribute, unsigned int *value)
>>   {
>>      struct glx_context *gc = __glXGetCurrentContext();
>>
>> -   if (gc == NULL)
>> +   if (gc == NULL || gc == &dummyContext)
>>         return False;
>>
>>      return __glXQueryRendererInteger(gc->psc, attribute, value);
>> @@ -166,7 +166,7 @@ glXQueryCurrentRendererStringMESA(int attribute)
>>   {
>>      struct glx_context *gc = __glXGetCurrentContext();
>>
>> -   if (gc == NULL)
>> +   if (gc == NULL || gc == &dummyContext)
> Unless I'm missing something __glXGetCurrentContext() can never return
> NULL, right ?

This was my initial impression also ... but I'm not sure what happens 
here but unit tests (glx-test) for some reason completely explode if you 
take out the null check and try running 'make check'.

I think that is why also the dummyContext is introduced in those tests. 
I played around a bit with this change and linking phase with glx-test 
starts to print quite weird errors, actually complete unrelated ones, I 
guess because it cannot find the 'extern' declared 'dummyContext'. I'm 
not sure if this is actually problem with the tests rather than this code.


> In that case I see two possible solutions
>   - Swap all the NULL checks,  for == &dummyContext (some commit
> message would be great) or
>   - Drop the null checks in this patch, and explain why we do that only
> here (the xid and vtable are implicitly zeroed for dummyContext, thus
> things will just work).
>
> Leaning for the latter for stable as it's shorter. Although doing the
> former on top (for master alone) also sounds like a good idea imho.
>
>>         return False;
>>
>>      return __glXQueryRendererString(gc->psc, attribute);
>> diff --git a/src/glx/tests/query_renderer_unittest.cpp b/src/glx/tests/query_renderer_unittest.cpp
>> index 2f3c4ef..4c96260 100644
>> --- a/src/glx/tests/query_renderer_unittest.cpp
>> +++ b/src/glx/tests/query_renderer_unittest.cpp
>> @@ -40,6 +40,10 @@ struct attribute_test_vector {
>>   #define E(x) { # x, x }
>>
>>
>> +/* This is necessary so that we don't have to link with glxcurrent.c
>> + * which would require us to link with X libraries and what not.
>> + */
>> +struct glx_context dummyContext;
>>
> Perhaps we should initialize it (like in glxcurrent.c) for
> consistency, if anything else ?
>
> -Emil
>