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

[Daniel P. Berrange]

On Mon, Mar 07, 2011 at 03:06:18PM +0800, Daniel Veillard wrote:
> 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 at 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();
>  }

ACK

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|