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

[Tapani Pälli]

On 05/27/2016 08:16 PM, Emil Velikov wrote:
> On Friday, 27 May 2016, Tapani Pälli <tapani.palli at intel.com 
> <mailto: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

This patch should cover all instances where __glXGetCurrentContext was 
called and had a check.

>  - piglits (Ian)
>  - some mesa glx tests are crashing/failing (yourself mentioned that 
> iirc). I take it that things are fine now ?
>

Yes things are fine now as I moved the dummyContext to fake_glx_screen 
where it should have been, 'glx-test' unit tests get their context from 
there.

> Thanks
> Emil


// Tapani


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160529/d0646af1/attachment-0001.html>