[PATCH] xserver: Optional backtrace handler
Barry Scott
barry.scott at onelan.co.uk
Thu Sep 22 08:55:46 PDT 2011
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>
More information about the xorg-devel
mailing list