Hi all, Anyone got a chance to look further into this? (this is an old bug with several unsuccessful attempted fixes)
Chris, what do you think of the wrap test for the sequence that must remain in 32bits? Cheers, Olivier ----- Original Message ----- From: "Olivier Fourdan" <[email protected]> To: "Christian Linhart" <[email protected]>, [email protected] Sent: Tuesday, March 31, 2015 2:22:11 PM Subject: Re: [PATCH libX11] fix for Xlib 32-bit request number issues Hi Christian, On 27/03/15 15:50, Olivier Fourdan wrote: > On 21/03/15 15:32, Christian Linhart wrote: > [...] > > I am not a libX11 maintainer, so it's not really a review or anything, > it's just some comments :-) So I looked further into your patch (took me some time!) and came to the following ideas: 1. The changes in Xlib.h are not needed as we already have LONG64 defined for the same purpose and already used in different place in libX11. 2. As a result, the changes in Xlibint.h would rely on LONG64 being defined (or not) instead of X_UNSIGNED_LONG_IS_32BIT 3. For the _XAsyncErrorState's "min_sequence_number" and "max_sequence_number", if we cannot change those to be 64bit, then we ought to use a different technique as assigning these 64bit values will make no difference. Thankfully, these are used in comparison, so we can check for a wrap using something like that for the 32bit version: #define SEQUENCE_CMP(a,op,b) \ (((a op b) && (b - a op (UINT32_MAX >> 1))) || \ ((b op a) && ((UINT32_MAX >> 1) op a - b))) Which (if my maths are correct) is supposed to compare "a" and "b" within a 32bit floating window, which is more than enough, as I would not expect the 32bit wrapping to occur more than once between a request and its async reply. That macro is defined and used in XlibAsync.c 4. For the code that uses the standard _XAsyncErrorState struct, there is no need to use 64bit extended sequence number so the macro above would work without changing the code - The list of sources is this case is Fonts.c, ReconfWM.c and obviously XlibAsync.c 5. For others that use their own struct, these can use 64bit sequence number and therefore benefit from your change, that's GetWAttrs.c and IntAtom.c 6. There are some unnecessary changes in your patch that seem unrelate to the issue being addressed, like for example renaming "sent" as "xcb_sent" in require_socket() from src/xcb_io.c or splitting the for() loop in 4 lines in _XSend() from src/xcb_io.c - These are cosmetic changes and, if really needed, should be part of a separate patch IMHO. 7. These changes will rely on xcb exposing the 64bit sequence number, ie an API addition in libxcb, so you will need to update the libxcb required version in configure.ac: X11_REQUIRES='xproto >= 7.0.17 xextproto xtrans xcb >= 1.1.92' X11_EXTRA_DEPS="xcb >= 1.1.92" But obviously that needs to be done once the patches in libxcb are merged, so this is left out for now. tl;dr: I am attaching a "git diff" against current git master's head so you can see what I mean (code is usually easier than a lengthy explanation) - I leave it to you to decide and send a new patch with "git am" if you think it's correct (or at least moving in the right direction) - I don't want to step on your toes here, just trying to help :-) Cheers, Olivier _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
