Re: [libvirt] [PATCH 5/6] Add an an internal API for emergency dump of debug buffer

On Fri, Mar 04, 2011 at 08:53:55AM -0700, Eric Blake wrote: > On 03/04/2011 03:30 AM, Daniel Veillard wrote: > > virLogEmergencyDumpAll() allows to dump the content of the > > debug buffer from within a signal handler. It saves to all > > log file or stderr if none is found > > * src/util/logging.h src/util/logging.c: add the new API > > and cleanup the old virLogDump code > > * src/libvirt_private.syms: exports it as a private symbol > > > > > +void > > +virLogEmergencyDumpAll(int signum) { > > + int ret = 0, len; > > + char buf[100]; > > + > > + if (virLogLen == 0) > > + return; > > > > - if ((virLogLen == 0) || (f == NULL)) > > - return 0; > > virLogLock(); > > Is virLogLock async-signal-safe? I could not find, I'm afraid it's implementation dependant. I would expect the lock to be done in user space using an atomic test and set op and hence being safe at least on i386 and x86_64. The problem is that if we drop the lock we can crash if used on USR2 while another thread is legitimately running. > > + snprintf(buf, sizeof(buf) - 1, > > + "Caught signal %d, dumping internal log buffer:\n", signum); > > snprintf is _not_ safe; it can call malloc. We probably ought to use a > manual decimal-to-string conversion loop instead. Even better I think a case on signum and outputting the signal description is even more useful, and simpler. > > + buf[sizeof(buf) - 1] = 0; > > + virLogDumpAllFD(buf, strlen(buf)); > > + snprintf(buf, sizeof(buf) - 1, "\n\n ====== start of log =====\n\n"); > > Why snprintf here, rather than strcpy? pure lazyness. I'm suggesting the following patch to fix those 2 issues, it get rids of the temporary buffer which was used to compute the length, better done in the logging routine directly. It also get rid of ret variable which was never used :-\ I'm also removing the early virLogLen == 0 test because we should at least log the fact we got the signal, even if the buffer may be empty. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

diff --git a/src/util/logging.c b/src/util/logging.c index e39e6c5..956e046 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -30,6 +30,7 @@ #include <sys/stat.h> #include <fcntl.h> #include <unistd.h> +#include <signal.h> #if HAVE_SYSLOG_H # include <syslog.h> #endif @@ -283,6 +284,9 @@ static void virLogStr(const char *str, int len) { static void virLogDumpAllFD(const char *msg, int len) { int i, found = 0; + if (len <= 0) + len = strlen(msg); + for (i = 0; i < virLogNbOutputs;i++) { if (virLogOutputs[i].f == virLogOutputToFd) { int fd = (long) virLogOutputs[i].data; @@ -308,24 +312,38 @@ static void virLogDumpAllFD(const char *msg, int len) { */ void virLogEmergencyDumpAll(int signum) { - int ret = 0, len; - char buf[100]; - - if (virLogLen == 0) - return; + int len; virLogLock(); - snprintf(buf, sizeof(buf) - 1, - "Caught signal %d, dumping internal log buffer:\n", signum); - buf[sizeof(buf) - 1] = 0; - virLogDumpAllFD(buf, strlen(buf)); - snprintf(buf, sizeof(buf) - 1, "\n\n ====== start of log =====\n\n"); - virLogDumpAllFD(buf, strlen(buf)); + switch (signum) { + case SIGFPE: + virLogDumpAllFD( "Caught signal Floating-point exception", -1); + break; + case SIGSEGV: + virLogDumpAllFD( "Caught Segmentation violation", -1); + break; + case SIGILL: + virLogDumpAllFD( "Caught illegal instruction", -1); + break; + case SIGABRT: + virLogDumpAllFD( "Caught abort signal", -1); + break; + case SIGBUS: + virLogDumpAllFD( "Caught bus error", -1); + break; + case SIGUSR2: + virLogDumpAllFD( "Caught User-defined signal 2", -1); + break; + default: + virLogDumpAllFD( "Caught unexpected signal", -1); + break; + } + virLogDumpAllFD(" dumping internal log buffer:\n", -1); + virLogDumpAllFD("\n\n ====== start of log =====\n\n", -1); while (virLogLen > 0) { if (virLogStart + virLogLen < LOG_BUFFER_SIZE) { virLogBuffer[virLogStart + virLogLen] = 0; virLogDumpAllFD(&virLogBuffer[virLogStart], virLogLen); - ret += virLogLen; virLogStart += virLogLen; virLogLen = 0; } else { @@ -336,8 +354,7 @@ virLogEmergencyDumpAll(int signum) { virLogStart = 0; } } - snprintf(buf, sizeof(buf) - 1, "\n\n ====== end of log =====\n\n"); - virLogDumpAllFD(buf, strlen(buf)); + virLogDumpAllFD("\n\n ====== end of log =====\n\n", -1); virLogUnlock(); }