On Fri, Apr 13, 2012 at 11:22:14AM +0200, Michal Suchanek wrote: > On 13 April 2012 03:09, Peter Hutterer <[email protected]> wrote: > > On Wed, Apr 11, 2012 at 06:33:26PM -0700, Chase Douglas wrote: > >> On 04/11/2012 04:53 PM, Peter Hutterer wrote: > >> > On Mon, Apr 09, 2012 at 11:17:26AM -0700, Chase Douglas wrote: > >> >> This new patch set includes a few fixes to the first and many more fixed > >> >> logging paths. All paths in the server that occur when it encounters a > >> >> fatal signal are now handled properly. > >> >> > >> >> I do not think this affects the crash I have been seeing in the Ubuntu > >> >> server. I tracked that issue down to a bug in the synaptics input > >> >> module. Since the only known issue is that running the server under > >> >> valgrind may fail if messages are logged inappropriately, I suggest we > >> >> not apply this to any stable branches. > >> >> > >> >> The full tree is still maintained at: > >> >> > >> >> http://cgit.freedesktop.org/~cndougla/xserver/log/?h=signal-logging > >> >> > >> >> Thanks! > >> > > >> > Looks good and this is certainly needed. I do have a few comments though: > >> > > >> > - I'd really like to see some tests of these new functions. Should be > >> > easy > >> > enough to write, given that they're all exposed and you can use strlen, > >> > strcmp, etc. to compare results. > >> > Plus, you should be able to test them inside a signal handler too with > >> > little effort. > >> > >> Some of the functions definitely could use tests, like strlen_sigsafe(). > >> I'll add some. > >> > >> However, I don't know if it's possible to add tests for behavior inside > >> a signal handler. For a test to be useful it must be repeatable and fail > >> every time it's wrong. Usually signal context corruption is > >> indeterminate. Maybe it could be repeatable with the use of valgrind, > >> but even then I'm not 100% sure. > >> > >> > - AFAICT, every single caller to LogMessageVerbSigSafe uses -1 as > >> > verbosity. > >> > How about a LogMessageSigSafe wrapper? > >> > >> There are a few places where I replaced non-ErrorF calls, where the > >> verbosity remains 1 instead of -1. > >> > >> I thought about creating a non-verb wrapper, but I didn't really have > >> much of an interest in reproducing all the different logging macros and > >> variations we currently have. The idea is that logging in signal context > >> should be fairly infrequent, and if you need it then you can get along > >> with all the args. > > > > It should be infrequent, but any error between ReadInput and mieqEnqueue > > will > > require it. That may not happen often, but the code still needs to be there. > > So a sensible API is something we should definitely aim for. > > > >> Secondarily, what would you default to? ErrorF uses -1, but LogMessage > >> uses 1. > > > > There's a reason that ErrorF is different (albeit just a wrapper) to > > LogMessage - it makes the code more obvious. A LogMessageVerb(..., -1, ...) > > is much less obvious and grep-able than ErrorF(). So at least having > > ErrorFSigSafe() is a good thing to have. It would make any new code stick > > out. "Hey, it's ErrorFSigSafe up there, but ErrorF here - one of them is > > wrong". > > > >> > - the API is rather shocking, replacing one vararg function with a > >> > number of > >> > mixed calls makes it really hard to grep for the message. > >> > a poor man's printf parsing that searches for %d, %s and %x would help > >> > here. > >> > >> Variadic argument implementation is some of the blackest magic of C > >> code, and varies a great deal even between architectures of glibc. I > >> don't know if we can be sure that variadic arguments will always be > >> signal safe. They aren't mentioned in the POSIX signal(7) man page as > >> safe functions. > >> > >> I doubt we would actually support Interix SUA, for example, but it is > >> POSIX compliant and its man page says va_* functions are not signal > >> safe: http://www.suacommunity.com/man/3/varargs.3.html. > > > > yeah, unfortunately that's the only documentation I found too, nothing else > > seems to hints at whether it's signal safe or not. > > > ... > > typedef union { > > uint32_t ui; > > int32_t i; > > char *c; > > const char *str; > > void *ptr; > > } log_param_t; > > > > /* Log a message using only signal safe functions. */ > > void > > LogMessageVerbSigSafe(MessageType type, int verb, const char *message, > > log_param_t* param) > ... > > Instead of varargs, the callers need to supply a log_param_t array, which is > > painful, but hopefully better than splitting log messages over several > > commands. > > > I don't think you can initialize an array of log_param_t sanely in > standard C. Unless you are going to have gcc as hard dependency this > is probably going to be more painful than just using multiple > statements.
I was under the impression that the union is the size of the biggest member, so 4 or 8 bytes in this case, depending on sizeof(void*). Isn't that the case here? Cheers, Peter _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
