<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd"> <html><head><meta name="qrichtext" content="1" /><style type="text/css"> p, li { white-space: pre-wrap; } </style></head><body style=" font-family:'Monospace'; font-size:10pt; font-weight:400; font-style:normal;"> Is there anything else I need to do to make this patch acceptable for inclusion in Xorg? Barry On Thursday 29 September 2011 11:21:08 Barry Scott wrote: > It is useful to be able to run an external program to analyse > a crashed server process. The server will run a user supplied > program in a subprocess of the server. > > The subprocess is created when the server starts up so that all > resources needed to create the subprocess are available. It is not > always possible to start up a process from within a crashed server. > For example if the malloc heap is corrupt fork will not work. > > If the server crashes a "crash" message is sent to the handler on > its stdin. The handler can then respond in whatever way it wishes. > If the handler wants the server to continue, return from > xorg_backtrace, it must write a line to its stdout. > > When then server exits normally the handler is sent the "exit" > message just before the server exits. > > Signed-off-by: Barry Scott <barry.scott@onelan.co.uk> > Reviewed-by: Simon Farnsworth <simon.farnsworth@onelan.co.uk> > --- > change from V3: > * close backtrace_handler_stdin in child process > > change from V2: > * restore trailing NL in main.c > * beef up comments to make it clea how loops are exited > when the child process dies > * Use _X_HIDDEN for new symbols that are not to be exported > > change from V1: > > * check for EAGAIN and EINTR on calls to read(), write() and waitpid() > * put -backtrace into lexical position in the command parser and usage > * fix coding style white space and condition ordering > * update man page with more details > * call xorg_cleanup_backtrace() from FatalError() > > dix/main.c | 5 ++ > include/os.h | 3 + > man/Xserver.man | 20 +++++++++ > os/backtrace.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++++- > os/log.c | 3 + > os/utils.c | 8 ++++ > 6 files changed, 159 insertions(+), 3 deletions(-) > > diff --git a/dix/main.c b/dix/main.c > index 16575ce..3326e78 100644 > --- a/dix/main.c > +++ b/dix/main.c > @@ -147,6 +147,8 @@ int main(int argc, char *argv[], char *envp[]) > > ProcessCommandLine(argc, argv); > > + xorg_init_backtrace(); > + > alwaysCheckForInput[0] = 0; > alwaysCheckForInput[1] = 1; > while(1) > @@ -354,6 +356,9 @@ int main(int argc, char *argv[], char *envp[]) > free(ConnectionInfo); > ConnectionInfo = NULL; > } > + > + xorg_cleanup_backtrace(); > + > return 0; > } > > diff --git a/include/os.h b/include/os.h > index a553f57..00336e5 100644 > --- a/include/os.h > +++ b/include/os.h > @@ -541,6 +541,9 @@ extern _X_EXPORT void ErrorF(const char *f, ...) _X_ATTRIBUTE_PRINTF(1,2); > extern _X_EXPORT void Error(const char *str); > extern _X_EXPORT void LogPrintMarkers(void); > > +extern _X_HIDDEN void xorg_init_backtrace(void); > extern _X_EXPORT void xorg_backtrace(void); > +extern _X_HIDDEN void xorg_cleanup_backtrace(void); > +extern _X_HIDDEN const char *BacktraceHandlerPath; > > #endif /* OS_H */ > diff --git a/man/Xserver.man b/man/Xserver.man > index f743912..9def022 100644 > --- a/man/Xserver.man > +++ b/man/Xserver.man > @@ -100,6 +100,9 @@ specifies a file which contains a collection of authorization records used > to authenticate access. See also the \fIxdm\fP(1) and > \fIXsecurity\fP(__miscmansuffix__) manual pages. > .TP 8 > +.B \-backtrace \fIbacktrace-handler-path\fP > +specifies the path to the optional backtrace handler program. See \fBBACKTRACE HANDLER\fP below. > +.TP 8 > .B \-br > sets the default root window to solid black instead of the standard root weave > pattern. This is the default unless -retro or -wr is specified. > @@ -530,6 +533,23 @@ the following font path: > /usr/share/fonts/default/ghostscript > .fi > > +.SH BACKTRACE HANDLER > + > +The optional backtrace handler program will be run in a subprocess when > +the server starts. The handler is invoked with argv[1] set to the PID > +of the server. > +.PP > +The handler is expected to read a line from stdin that will tell the > +handler what has happened to the server. > +.PP > +When the server is about to exit normally "exit\\n" is sent to the handler. > +.PP > +If the server crashes "crash\n" is sent to the handler and the server will > +wait for a line to be written out on the handler's stdout. This causes > +the server to return from the xorg_backtrace function. Typically after calling > +xorg_backtrace additional error messages are logged and the server shuts down. > +If you do not want the server to continue then kill it. > + > .SH FILES > .TP 30 > .I /etc/X\fBn\fP.hosts > diff --git a/os/backtrace.c b/os/backtrace.c > index 7ca6dab..bcdcbf6 100644 > --- a/os/backtrace.c > +++ b/os/backtrace.c > @@ -28,6 +28,11 @@ > #include "os.h" > #include "misc.h" > > +#include <unistd.h> > +#include <errno.h> > +#include <sys/types.h> > +#include <sys/wait.h> > + > #ifdef HAVE_BACKTRACE > #ifndef _GNU_SOURCE > #define _GNU_SOURCE > @@ -35,7 +40,7 @@ > #include <dlfcn.h> > #include <execinfo.h> > > -void xorg_backtrace(void) > +void xorg_builtin_backtrace(void) > { > void *array[64]; > const char *mod; > @@ -180,7 +185,7 @@ static int xorg_backtrace_pstack(void) { > > # if defined(HAVE_PSTACK) || defined(HAVE_WALKCONTEXT) > > -void xorg_backtrace(void) { > +void xorg_builtin_backtrace(void) { > > ErrorF("\nBacktrace:\n"); > > @@ -207,7 +212,119 @@ void xorg_backtrace(void) { > # else > > /* Default fallback if we can't find any way to get a backtrace */ > -void xorg_backtrace(void) { return; } > +void xorg_builtin_backtrace(void) { return; } > > # endif > #endif > + > +const char *BacktraceHandlerPath = NULL; > + > +static int backtrace_handler_stdin; > +static int backtrace_handler_stdout; > +static pid_t child_pid; > + > +static const char cmd_crash[] = "crash\n"; > +static const char cmd_exit[] = "exit\n"; > + > +static void report_init_error(const char *msg) { > + /* prevent any further use of the handler */ > + BacktraceHandlerPath = NULL; > + FatalError("%s - errno %d", msg, errno); > +} > + > +void xorg_init_backtrace(void) { > + char pid_buffer[32]; > + int pipe_fds[2]; > + int child_stdin; > + int child_stdout; > + > + if (!BacktraceHandlerPath) > + return; > + > + if (pipe(pipe_fds) != 0) > + report_init_error("failed to create pipe"); > + backtrace_handler_stdin = pipe_fds[1]; > + child_stdin = pipe_fds[0]; > + > + if (pipe(pipe_fds) != 0) > + report_init_error("failed to create pipe"); > + backtrace_handler_stdout = pipe_fds[0]; > + child_stdout = pipe_fds[1]; > + > + /* server pid will be passed in argv[1] */ > + snprintf( pid_buffer, sizeof(pid_buffer), "%d", getpid()); > + > + child_pid = fork(); > + if (child_pid < 0) { > + report_init_error("failed to fork"); > + } else { > + if (child_pid == 0) { > + /* setup stdin, stdout to point to the pipes */ > + dup2(child_stdin, STDIN_FILENO); > + dup2(child_stdout, STDOUT_FILENO); > + > + /* close all other fds to the pipes */ > + close(child_stdin); > + close(child_stdout); > + close(backtrace_handler_stdin); > + close(backtrace_handler_stdout); > + > + execl(BacktraceHandlerPath, BacktraceHandlerPath, pid_buffer, NULL); > + exit(1); > + } else { > + if (close(child_stdin) != 0) > + report_init_error("failed to close pipe fd"); > + if (close(child_stdout) != 0) > + report_init_error("failed to close pipe fd"); > + } > + } > +} > + > +void xorg_backtrace(void) { > + if (BacktraceHandlerPath) { > + int status; > + > + /* check that the handler process is still running */ > + if (waitpid(child_pid, &status, WNOHANG) == 0) { > + char buffer[1]; > + > + /* tell backtrace handler xorg has crashed */ > + while (write(backtrace_handler_stdin, cmd_crash, sizeof(cmd_crash ) - 1) < 0) > + if (errno != EAGAIN && errno != EINTR) > + break; > + > + /* exit to caller after backtrace handler writes anything to stdout */ > + while (read(backtrace_handler_stdout, buffer, sizeof(buffer)) < 0) > + if (errno != EAGAIN && errno != EINTR) > + break; > + > + } else { > + /* handler process has quit? run builtin handler */ > + xorg_builtin_backtrace(); > + } > + } else { > + xorg_builtin_backtrace(); > + } > +} > + > +void xorg_cleanup_backtrace(void) { > + if (BacktraceHandlerPath) { > + int status; > + > + BacktraceHandlerPath = NULL; > + > + /* tell backtrace handler xorg is about to exit cleanly > + EPIPE is returned if the child process closes the pipe > + (which happens when the process exits) and the loop will exit */ > + while (write(backtrace_handler_stdin, cmd_exit, sizeof(cmd_exit ) - 1) < 0) > + if (errno != EAGAIN && errno != EINTR) > + break; > + > + /* If the child has already exited waitpid return with ECHILD > + and the loop will exit */ > + while (waitpid(child_pid, &status, 0) < 0) > + if (errno != EINTR) > + break; > + > + } > +} > diff --git a/os/log.c b/os/log.c > index f519762..7b3409a 100644 > --- a/os/log.c > +++ b/os/log.c > @@ -523,6 +523,9 @@ FatalError(const char *f, ...) > va_list args; > static Bool beenhere = FALSE; > > + /* make sure any backtrace handler process is told to exit */ > + xorg_cleanup_backtrace(); > + > if (beenhere) > ErrorF("\nFatalError re-entered, aborting\n"); > else > diff --git a/os/utils.c b/os/utils.c > index 36cb46f..78c39e6 100644 > --- a/os/utils.c > +++ b/os/utils.c > @@ -470,6 +470,7 @@ void UseMsg(void) > ErrorF("-ac disable access control restrictions\n"); > ErrorF("-audit int set audit trail level\n"); > ErrorF("-auth file select authorization file\n"); > + ErrorF("-backtrace string path of program to run to handle a server crash\n"); > ErrorF("-br create root window with black background\n"); > ErrorF("+bs enable any backing store support\n"); > ErrorF("-bs disable any backing store support\n"); > @@ -616,6 +617,13 @@ ProcessCommandLine(int argc, char *argv[]) > else > UseMsg(); > } > + else if ( strcmp( argv[i], "-backtrace") == 0) > + { > + if(++i < argc) > + BacktraceHandlerPath = argv[i]; > + else > + UseMsg(); > + } > else if ( strcmp( argv[i], "-br") == 0) ; /* default */ > else if ( strcmp( argv[i], "+bs") == 0) > enableBackingStore = TRUE; > </body></html>