Re: [libvirt] [PATCH 7/7] Remove global log buffer feature entirely

On 03/03/2014 12:18 PM, Daniel P. Berrange wrote: I still wonder if there is value on keeping the ring buffer for when logging is not stderr. That is, if we dump to syslog by default but ALSO dump the ring buffer to stderr, it may be easier to find what got logged by reading stderr rather than having to hunt for the syslog. But that's a weak argument. If we commit to patch 1, then the logging ring really is duplicated overhead, and I'm fine ditching it. You may want to get DV's opinion, but I'm fine with this patch going in on the premise that patch 1 made sense. On the other hand, if patch 1 meets resistance, should we look at ways to minimize printf() overhead? For example, a LOT of our overhead is tied to the fact that we malloc() a lot when constructing the log string. It may be possible to fine-tune things so that most (if not all) log messages get computed into a static buffer (gnulib's asnprintf is cool in how it avoids malloc in the common case if you provide a large enough buffer, while still being robust to larger strings). It won't kill as much overhead as what you killed by completely ditching the ring buffer, but may still be enough improvement to merit keeping the ring buffer around. I'm not tied to the ring buffer - I still think that targetted logging is easier to sift through than the signal-to-noise ratio of a ring buffer dump. I'm just raising the question to make sure we think of the issues before applying patch 1 and losing what might still be a useful last-ditch debug aid for hard-to-reproduce crashes. Wow - quite a lot tied up in the log ring buffer. Does augeas gracefully handle an option that is no longer used by current libvirtd, but which mattered to older versions? That is, I think you still have to support the option in the .conf file, and document that it is now ignored, but guarantee that having it in the .conf file doesn't cause a syntax error. That is, I don't think you can safely delete this line. And that you may need to mention that this variable is present but ignored for backward compatibility, rather than deleting it altogether. Same story. We may not be dumping the ring buffer any more, but I _still_ think it's worth keeping this signal handler and dumping a message that we detected a fatal crash, as well as pointing the user to go read the contents of the logging files (and/or rerun libvirtd with more logging enabled). In other words, delete the ring buffer, but DON'T delete this last-ditch message to the user. Deleting USR2 handling makes sense, though. Again, I still think this function is useful - not to dump the ring buffer, but to mention that we are going down and to check the logs and give an address where to report the bug. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org