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/
Attachments:
- logging_cleanup.patch (text/plain — 2.7 KB)