Re: [PATCH 0/7] tools/kvm_stat: add logfile support

Stefan Raspl
2020-03-29T11:22:22Z

On 2020-03-24 11:32, Paolo Bonzini wrote:
> On 24/03/20 09:26, Stefan Raspl wrote:
>> To be able to make use of the logfiles, we'd need to have the heading appear at
>> the top of each of the files.
>> Couldn't find much info on how logrotate works internally, but from what I
>> gathered, it seems it moves out the current logfile e.g. /var/log/kvm.log to
>> become /var/log/kvm.log.1, and sends a SIGHUP to kvm_stat so that it re-opens
>> /var/log/kvm.log - which would then start out with a header again.
>> That should work, but can you confirm that this is what you're suggesting?
>> If so: Keep the current semantics for the original logging mode, where we have
>> the heading printed every 20 lines? I would assume so, as that format is better
>> suited for console logs, but just in case you wanted that changed...
>
> Yes to all. :)

I wrote a respective patch and tried it out, and found this approach not to be
workable for a number of reasons:
- The implicit buffering that takes place when redirecting output of kvm_stat in
logging mode to a file messes up the format: logrotate moves the files on the
disks, but there might be still data buffered that hasn't been written out
yet. The SIGHUP triggers a new header to be written with the patch I came up
with, but that header would sometimes appear only after a couple of lines
written to the file, which messes up the format. Flushing stdout in the signal
handler won't help, either - it's already too late by then.
- When restarting kvm_stat logging (e.g. after a reboot), appending to an
existing file should suppress a new header from being written, or it would end
up somewhere in the middle of the file, messing up the format. kvm_stat
doesn't know that or where its output is redirected to, so no chance of
suppressing it within kvm_stat. We would probably require some kind of wrapper
script (and possibly an extra cmd-line option to suppress the header on
start).
- I was surprised to realize that SIGHUP is actually not part of logrotate -
one has to write that manually into the logrotate config as a postrotate...
and I'll openly admit that writing a respective killall-command that aims at a
python script doesn't seem to be trivial...

Any idea how to address these issues?

As much as I sympathize with re-use of existing components, I'd like to point
out that my original patch was also re-using existing python code for rotating
logs, and made things just _so_ much easier from a user perspective.

Ciao,
Stefan