[PATCH] xserver: Optional backtrace handler

[Simon Farnsworth]

Review is inline:

On Wednesday 21 September 2011, Barry Scott <barry.scott at onelan.co.uk> 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
                                                 ^      ^^^^^^^^
                            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.

"If the handler wants the server to return from xorg_backtrace, it
must write a line to its stdout". I'm not sure that the server can
sensibly continue after xorg_backtrace is called.

> 
> 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(-)
> 
<snip>
> -
> 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);

I'm not sure _X_EXPORT is appropriate here - given that these
functions are meant to be called by main and main alone, would
_X_HIDDEN be better?

> +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
<snip>
> -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.

"the server will wait for a line to *be* written out on the *handler's* 
stdout".

I would add a bit here about what you expect the handler to do to the
server, and what happens if it replies; my guess would be that the
server would attempt to continue normally unless the handler kills it,
but in fact, when the handler says "go", the server will attempt a
clean exit.

> +.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
<snip>
> @@ -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))

Unusual coding style for the X server - I'd expect to see:
if (pipe(pipe_fds) != 0)

> +        report_init_error("failed to create pipe");
> +    backtrace_handler_stdin = pipe_fds[1];
> +    child_stdin = pipe_fds[0];
> +
> +    if (0!=pipe(pipe_fds))

And again here.

> +        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 */

I'd make fd plural here - "close all other fds"

> +            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))

Again odd - I'd expect:
if (close(child_stdin) != 0 )

> +                report_init_error("failed to close pipe fd");
> +            if (0!=close(child_stdout))

And again.

> +                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)) {

if (waitpid(child_pid, &status, WNOHANG) == 0) {

> +            char buffer[1];
> +            /* tell backtrace handler xorg has crashed */
> +            write(backtrace_handler_stdin, cmd_crash, sizeof( cmd_crash 
)-1);

Spaces in sizeof() are odd - "sizeof(cmd_crash) - 1" is closer to X.org style.

> +            /* exit to caller after backtrace handler writes anything to 
stdout */
> +            read(backtrace_handler_stdout, buffer, sizeof( buffer ));

And again.

> +        } 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);

Again with the extra spaces in sizeof.

You've asked the backtracer to die - it would be polite to reap the
resulting zombie with waitpid(). Also, once you've cleaned it up, set
BacktraceHandlerPath to NULL, just in case we enter this path during
shutdown.

A general thing in all of the above; read() and write() can return an
error (EPIPE, EINVAL, EINTR all seem plausible). I would expect you to
retry the operation on EINTR, and assume the external backtracer has
died already on any other return value.

One final point - if the X server dies via FatalError, nothing
terminates the external backtracer. It would be polite to tell it to
go away.

The rest looks good - I'll review the next version when it appears.

-- 
Simon Farnsworth
Software Engineer
ONELAN Limited
http://www.onelan.com/