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" <[email protected]> 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 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
