<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd"> <html><head><meta name="qrichtext" content="1" /><style type="text/css"> p, li { white-space: pre-wrap; } </style></head><body style=" font-family:'Monospace'; font-size:10pt; font-weight:400; font-style:normal;"> On Thursday 22 September 2011 11:56:09 Simon Farnsworth wrote: > Review is inline: > > On Wednesday 21 September 2011, Barry Scott <barry.scott@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@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 </body></html>