[PATCH-V2] xserver: Optional backtrace handler

[Simon Farnsworth]

Review inline. Commit message is fine this time, so snipped.

On Thursday 22 September 2011, Barry Scott <barry.scott at onelan.co.uk> wrote:
<snip>
> 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;
>  }
> -

Nitpick: why remove this blank line?

> 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;
>

I still think these ought to be _X_HIDDEN or _X_INTERNAL, rather than
_X_EXPORT - they shouldn't be called by modules, for example.

>  #endif /* OS_H */

<snip>
> diff --git a/os/backtrace.c b/os/backtrace.c
> index 7ca6dab..c399a1f 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,114 @@ 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;
> +
> +        /* tell backtrace handler xorg is about to exit cleanly */
> +        while (write(backtrace_handler_stdin, cmd_exit, sizeof(cmd_exit ) - 
1) < 0)
> +            if (errno != EAGAIN && errno != EINTR)
> +                break;
> +
> +        while (waitpid(child_pid, &status, 0) < 0)
> +            if (errno != EINTR)
> +                break;
> +
> +        BacktraceHandlerPath = NULL;

Nitpick: I'd set BacktraceHandlerPath to NULL at the beginning of this
code block (before the write and waitpid). I know it makes very little
difference (since X is supposed to be single-threaded), but given that
we're in trouble if xorg_backtrace is ever called, I'd rather do it as:

if (BacktraceHandlerPath) {
	int status;

	BacktraceHandlerPath = NULL;

to maximise the chance of getting a backtrace if
xorg_cleanup_backtrace is called immediately before a signal or
unexpected thread (maybe one from AIGLX) calls xorg_backtrace.

Also, it took me a few seconds to convince myself that we'd be able to
escape an endless loop if the writes never succeeded - a quick comment
to point out that we are guaranteed EPIPE on the reads and writes, and
ECHILD on the waitpids if the handler dies would be nice.

> +    }
> +}
<snip>

Apart from the nitpicks above, it looks good to me; with them fixed, feel free 
to add my reviewed-by:

Reviewed-by: Simon Farnsworth <simon.farnsworth at onelan.co.uk>

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