[Mesa-dev] [PATCH 3/3] xlib: use X error handler to catch XShmQueryVersion() failure

[Brian Paul]

On 01/11/2012 08:13 AM, Adam Jackson wrote:
> On Tue, 2012-01-10 at 19:46 -0700, Brian Paul wrote:
>> From: Brian Paul<brianp at vmware.com>
>>
>> This is a follow-on to the previous commits.  It seems that
>> XShmQueryVersion() can trigger an X error after the first X
>> connection is closed and we start using a new connection.
>
> I assume this is when you switch which Display you're talking to as
> well.

Yes.  It's really weird.  I looked at the sources for the XShm 
extension code and I didn't see anything obviously wrong there.

If I disabled all use of XShm in the Mesa code the test app worked 
fine.  Also, If I hard coded the check_for_xshm() function to just 
always return true, things ran normally as well.

So the X error handler is a band-aid over the problem, but it seems to 
do the job.

I guess another "solution" would be to simply remove the call to 
XShmQueryVersion() and assume that if XQueryExtension("MIT-SHM") 
passes then the version is acceptable (it's been 1.1 forever, AFAIK). 
  What do you think about that?



>> +static int
>> +xshm_query_version_err_handler( XMesaDisplay* dpy, XErrorEvent* xerr )
>> +{
>> +   /* If this function gets called, it's because XShmQuerryVersion()
>> +    * has failed.
>> +    */
>> +   XShmErrorFlag = True;
>> +   return 0;
>> +}
>
> Not safe against a process using multiple Display connections, which is
> admittedly quite rare.  There's a few instances of that kind of bug
> already though, I wouldn't call it a blocker.
>
>> +      /* Install X error handler before calling XShmQueryVersion() to catch
>> +       * a spurious X error.
>> +       */
>> +      XShmErrorFlag = False;
>> +      old_handler = XSetErrorHandler(xshm_query_version_err_handler);
>> +      ok = XShmQueryVersion( display,&major,&minor,&pixmaps );
>> +      XSetErrorHandler(old_handler);
>> +      if (XShmErrorFlag)
>> +         ok = False;
>> +
>> +      if (ok) {
>
> Not thread-safe, though it's tough to hit.  You want an XLockDisplay
> before the first XSetErrorHandler and the matching XUnlockDisplay after
> the second, otherwise your handler can get called for errors on other
> threads.

I guess I'm not too concerned about that for the time being.  As you 
said, there's other instances of multi-display and thread safety 
issues already.

-Brian