[PATCH] xserver: Optional backtrace handler

[Barry Scott]

On Thursday 22 September 2011 11:56:09 Simon Farnsworth wrote:
> 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

fixed in next patch.

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

The design leaves that decision upto the handler. In some cases the
call to xorg_backtrace can be continued from in other as we have seen
it cannot.

The existing Xorg code typically calls FatalError after xorg_backtrace
returns and runs its logic.

> 
> > 
> > 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".

Fixed in next patch

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

Done.

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

done.

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

done.

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

done.

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

done.

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

done.

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

done.

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

done.

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

done.

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

done.

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

Both done.

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

I check for EINTR and EAGAIN in all calls and retry.

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

I have added a xorg_cleanup_backtrace call to FatalError and
made xorg_cleanup_backtrace safe to call multiple times.

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

Updated patch will follow.

Barry
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110922/4abc6ac2/attachment-0001.htm>