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

[Tapani Pälli]

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?

> On 02/11/2016 04:04 AM, Tapani Pälli 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)
>>         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;
>>
>>   static bool got_sigsegv;
>>   static jmp_buf jmp;
>>
>