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/
> [email protected] | 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 :|
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
