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

[Ian Romanick]

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.

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.

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.

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;
>