[PATCH-V3] xserver: Optional backtrace handler
Jeremy Huddleston
jeremyhu at apple.com
Wed Sep 28 10:37:04 PDT 2011
After some thought, I think it's a good idea to not wait for the handler to exit. It may want to do some extra processing that it doesn't need the process hanging around for.
You probably meant one of these to be backtrace_handler_stdin:
> + close(backtrace_handler_stdout);
> + close(backtrace_handler_stdout);
I'd like to see a followup to set BacktraceHandlerPath from xorg.conf
On Sep 28, 2011, at 3:09 AM, 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 the server exits normally the handler is sent the "exit"
> message just before the server exits.
>
> Signed-off-by: Barry Scott <barry.scott at onelan.co.uk>
> Reviewed-by: Simon Farnsworth <simon.farnsworth at onelan.co.uk>
> ---
> 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..f9114eb 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_stdout);
> + 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;
> --
> 1.7.6
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>
More information about the xorg-devel
mailing list