[PATCH libX11 v2 1/4] Add _XGetRequest as substitute for GetReq/GetReqExtra

[Peter Hutterer]

On Wed, Nov 02, 2011 at 11:23:55AM -0700, Jamey Sharp wrote:
> On Wed, Nov 02, 2011 at 05:32:24AM -0700, Dan Nicholson wrote:
> > On Oct 27, 2011 4:26 AM, "Peter Hutterer" <peter.hutterer at who-t.net> wrote:
> > > On Thu, Oct 27, 2011 at 10:41:06AM +0200, walter harms wrote:
> > > > Am 27.10.2011 09:55, schrieb Jamey Sharp:
> > > > > On Thu, Oct 27, 2011 at 09:15:54AM +0200, walter harms wrote:
> > > > >> Does it make sense to continue here ?
> > > > >> perhaps you want a add a return NULL ?
> > > > >
> > > > > It doesn't make sense to continue, but there's no way to report the
> > > > > error that any caller can handle. If you return NULL here, the caller is
> > > > > guaranteed to segfault.
> > > >
> > > > what is bad with segfault ? the macro version would more or less make
> > > > sure that len % 4 is 0 by using sizeof. Everybody else has a serious problem.
> > > > Better you stop here (by returning NULL) than at a random place.
> > >
> > > all the existing code still uses the macros so the same assumptions are
> > > true. Only new code could use _XGetRequest directly and you'd hope that
> > > whoever writes that code runs it at least once to see the error message.
> > 
> > Drive by reviewing ...
> > What about an assert? Than the user's incorrect program will at least
> > stop with a usefulish message. Seems better then a segfault.
> 
> That's a quite sensible-sounding answer, which I've tried in libX11 in
> the past. You or Walter are welcome to commit a follow-on patch that
> asserts or returns NULL, whichever you prefer, as long as you take
> responsibility for any bug reports that follow. :-) Keep in mind that
> distros on the whole are amazingly bad about forwarding bug reports
> upstream, so you'll need to monitor multiple distros' bug trackers
> yourself. Also be prepared for users you've never met cursing your name.
> Bleh. :-/
> 
> I really do agree that it's better to catch this sort of bug.
> Unfortunately, asserts and segfaults mean that the people who are most
> likely to encounter bugs are the least capable of dealing with them.
> 
> The best option, of course, is to ensure that no bug can occur. Peter,
> what about making the length argument take 4-byte units, dividing in the
> callers, and multiplying inside _XGetRequest? 

I had that in the first version but we cannot change GetReqExtra and that
takes bytes, not 4-byte units. And having two ways to get a request with a
specific size that take different units seemed more error-prone.
SIZEOF() returns bytes as well, so imo switching to a different unit to help
those potential future developers that don't read the documentation for the
macros isn't overly useful. It's not like the 4-byte alignment requirement
of the protocol is a secred either :)

Cheers,
  Peter

> The division will be
> constant-folded away, so there's no code-size cost in the callers. We
> could even round up to the nearest four-byte boundary for free, although
> that means that buggy extensions magically work when built with new
> headers but fail when built with old headers. Or throw something like
> the Linux kernel BUILD_BUG_ON macro into the GetReq macros...
> 
> Of course there's the further problem that _XGetRequest doesn't work for
> requests bigger than either the core maximum request size or the Xlib
> output queue size, whichever is smaller. Which raises the same questions
> of printf or assert or return NULL all over again. But I don't think
> this bike-shedding should block merging the patches.
> 
> Jamey