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

[Jamey Sharp]

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? 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20111102/8c807dde/attachment.pgp>