[PATCH] xserver: Optional backtrace handler
Jeremy Huddleston
jeremyhu at apple.com
Wed Sep 21 13:27:11 PDT 2011
I like the idea in theory, but I have a few requests:
1) Check for errors on the write. (eg: EAGAIN is not handled properly)
2) grammer:
> +wait for a line to written out on the handlers stdout.
should be:
> +wait for a line to be written out on the handlers stdout.
3) Why wait for the child to write back? I'd rather require the child exit after handling the report and just wait(2) for that.
4) Whether you read(2) or wait(2) from #3 above, we still have an issue where we block the shutdown path on the actions of another process. I'd feel much better if there were a timeout. Unfortunately, I don't see a POSIX way to do a wait(2) with a timeout from within a signal handler. (anyone else have thoughts?) ... I'd be fine polling it with waitpid(2) with WNOHANG and using nanosleep(2) to give a reasonable (5 second? / configurable?) timeout.
On Sep 21, 2011, at 08:54, Barry Scott wrote:
> It is useful to be able to run an external program to analyse
> a crashed server process. The server will run an user surplied
> 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 at onelan.co.uk>
> ---
> dix/main.c | 6 +++-
> include/os.h | 3 ++
> man/Xserver.man | 19 ++++++++++
> os/backtrace.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> os/utils.c | 8 ++++
> 5 files changed, 133 insertions(+), 4 deletions(-)
>
> diff --git a/dix/main.c b/dix/main.c
> index 16575ce..96b3f37 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,8 @@ 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..48debe9 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -542,5 +542,8 @@ extern _X_EXPORT void Error(const char *str);
> extern _X_EXPORT void LogPrintMarkers(void);
>
> extern _X_EXPORT void xorg_backtrace(void);
> +extern _X_EXPORT void xorg_init_backtrace(void);
> +extern _X_EXPORT void xorg_cleanup_backtrace(void);
> +extern const char *BacktraceHandlerPath;
>
> #endif /* OS_H */
> diff --git a/man/Xserver.man b/man/Xserver.man
> index f743912..4933375 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,22 @@ 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 written out on the handlers stdout.
> +.PP
> +
> +
> .SH FILES
> .TP 30
> .I /etc/X\fBn\fP.hosts
> diff --git a/os/backtrace.c b/os/backtrace.c
> index 7ca6dab..a897876 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,97 @@ 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 (0!=pipe(pipe_fds))
> + report_init_error("failed to create pipe");
> + backtrace_handler_stdin = pipe_fds[1];
> + child_stdin = pipe_fds[0];
> +
> + if (0!=pipe(pipe_fds))
> + 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 fd 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 (0!=close(child_stdin))
> + report_init_error("failed to close pipe fd");
> + if (0!=close(child_stdout))
> + 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 (0==waitpid(child_pid, &status, WNOHANG)) {
> + char buffer[1];
> + /* tell backtrace handler xorg has crashed */
> + write(backtrace_handler_stdin, cmd_crash, sizeof( cmd_crash )-1);
> + /* exit to caller after backtrace handler writes anything to stdout */
> + read(backtrace_handler_stdout, buffer, sizeof( buffer ));
> + } else {
> + /* handler process has quit? run builtin handler */
> + xorg_builtin_backtrace();
> + }
> + } else {
> + xorg_builtin_backtrace();
> + }
> +}
> +
> +void xorg_cleanup_backtrace(void) {
> + char buffer[1];
> + if (BacktraceHandlerPath) {
> + /* tell backtrace handler xorg is about to exit cleanly */
> + write(backtrace_handler_stdin, cmd_exit, sizeof( cmd_exit )-1);
> + }
> +}
> diff --git a/os/utils.c b/os/utils.c
> index 36cb46f..aeb18b8 100644
> --- a/os/utils.c
> +++ b/os/utils.c
> @@ -478,6 +478,7 @@ void UseMsg(void)
> ErrorF("-cc int default color visual class\n");
> ErrorF("-nocursor disable the cursor\n");
> ErrorF("-core generate core dump on fatal error\n");
> + ErrorF("-backtrace string path of program to run to handle a server crash\n");
> ErrorF("-dpi int screen resolution in dots per inch\n");
> #ifdef DPMSExtension
> ErrorF("-dpms disables VESA DPMS monitor control\n");
> @@ -653,6 +654,13 @@ ProcessCommandLine(int argc, char *argv[])
> {
> EnableCursor = FALSE;
> }
> + else if ( strcmp( argv[i], "-backtrace") == 0)
> + {
> + if(++i < argc)
> + BacktraceHandlerPath = argv[i];
> + else
> + UseMsg();
> + }
> else if ( strcmp( argv[i], "-dpi") == 0)
> {
> if(++i < argc)
> --
> 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
>
---
Jeremy Huddleston
Rebuild Sudan
- Board of Directors
- http://www.rebuildsudan.org
Berkeley Foundation for Opportunities in Information Technology
- Advisory Board
- http://www.bfoit.org
More information about the xorg-devel
mailing list