On Wed, Nov 02, 2011 at 05:32:24AM -0700, Dan Nicholson wrote: > On Oct 27, 2011 4:26 AM, "Peter Hutterer" <peter.hutte...@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
signature.asc
Description: Digital signature
_______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
