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

Paolo Bonzini
2020-03-30T10:43:59Z

On 29/03/20 13:22, Stefan Raspl wrote:
> 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.

I don't understand this. Unless you were using copytruncate, the
sequence should be:

- logrotate moves file A to B

- your file descriptor should point to B, so kvm_stat keeps writing to
file B

- you send the signal to kvm_stat

- kvm_stat closes file B, so all pending records are written

- kvm_stat reopens file A and writes the header.

If you have a race of some sort, try having the signal handler do
nothing but set a flag that is examined in the main loop.

> - 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).

You could stat the output file, and suppress the header if it is a
regular non-empty file. But it would be a problem anyway if the header
has changed since the last boot, which prompts the stupid and lazy
question: how does your series deal with this?

This one seems the biggest of the three problems to me.

> - 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...

This one is easy, put "ExecReload=/bin/kill -HUP $MAINPID" in the
systemd unit and use "systemctl reload kvm_stat.service" in the
postrotate command.

> 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.

I understand that, yes. My request was more about not requiring
kvm_stat-specific configuration than about reusing existing components,
though.

Paolo