On 2020-03-31 23:02, Paolo Bonzini wrote:
> On 31/03/20 22:00, Stefan Raspl wrote:
>> From: Stefan Raspl <raspl@xxxxxxxxxx>
>>
>> To integrate with logrotate, we have a signal handler that will re-open
>> the logfile.
>> Assuming we have a systemd unit file with
>> ExecStart=kvm_stat -dtc -s 10 -L /var/log/kvm_stat.csv
>> ExecReload=/bin/kill -HUP $MAINPID
>> and a logrotate config featuring
>> postrotate
>> /bin/systemctl restart kvm_stat.service
>
> Does reload work too?
Reload and restart work fine - any specific concerns or areas to look for?
> Regarding the code, I only have a few nits.
>
>
>> + f.write(frmt.get_banner())
>> + f.write('\n')
>> + else:
>> + print(frmt.get_banner())
>
> What about
>
> print(frmt.get_banner(), file=f or sys.stdout)
Yup, thx!
>> +
>> + def do_statline(opts):
>> + statline = frmt.get_statline(keys, stats.get())
>> + if len(statline) == 0:
>> + return False
>> + statline = datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline
>> + if opts.log_to_file:
>> + f.write(statline)
>> + f.write('\n')
>> + else:
>> + print(statline)
>
> ... and likewise here? (
Sure
>>
>> + return True
>> +
>> + do_banner(opts)
>> + banner_printed = True
>> while True:
>> try:
>> time.sleep(opts.set_delay)
>> - if line % banner_repeat == 0 and not banner_printed:
>> - print(frmt.get_banner())
>> + if signal_received:
>> + banner_printed = True
>> + line = 0
>> + f.close()
>> + do_banner(opts)
>> + signal_received = False
>> + if (line % banner_repeat == 0 and not banner_printed and
>> + not (opts.log_to_file and isinstance(frmt, CSVFormat))):
>> + do_banner(opts)
>> banner_printed = True
>> - statline = frmt.get_statline(keys, stats.get())
>> - if len(statline) == 0:
>> + if not do_statline(opts):
>> continue
>> - print(datetime.now().strftime("%Y-%m-%d %H:%M:%S") + statline)
>> line += 1
>> banner_printed = False
>> except KeyboardInterrupt:
>> break
>>
>> + if opts.log_to_file:
>> + f.close()
>
> "if f:"?
I'd argue the former is a lot more telling/easier to read - one of the downsides
of using extremely short variable names like 'f'.
>> +
>> +
>> +def handle_signal(sig, frame):
>> + global signal_received
>> +
>> + signal_received = True
>> +
>> + return
>> +
>>
>> def is_delay_valid(delay):
>> """Verify delay is in valid value range."""
>> @@ -1652,6 +1703,10 @@ Press any other key to refresh statistics immediately.
>> default=False,
>> help='run in logging mode (like vmstat)',
>> )
>> + argparser.add_argument('-L', '--log-to-file',
>> + type=str,
>> + help="like '--log', but logging to a file"
>> + )
>> argparser.add_argument('-p', '--pid',
>> type=int,
>> default=0,
>> @@ -1675,9 +1730,9 @@ Press any other key to refresh statistics immediately.
>> help='omit records with all zeros in logging mode',
>> )
>> options = argparser.parse_args()
>> - if options.csv and not options.log:
>> + if options.csv and not (options.log or options.log_to_file):
>> sys.exit('Error: Option -c/--csv requires -l/--log')
>
> "requires -l/-L"?
Yes!
>> - if options.skip_zero_records and not options.log:
>> + if options.skip_zero_records and not (options.log or options.log_to_file):
>> sys.exit('Error: Option -z/--skip-zero-records requires -l/--log')
>
> Likewise.
Of course.
>> try:
>> # verify that we were passed a valid regex up front
>> @@ -1758,7 +1813,9 @@ def main():
>> sys.stdout.write(' ' + '\n '.join(sorted(set(event_list))) + '\n')
>> sys.exit(0)
>>
>> - if options.log:
>> + if options.log or options.log_to_file:
>> + if options.log_to_file:
>> + signal.signal(signal.SIGHUP, handle_signal)
>> keys = sorted(stats.get().keys())
>> if options.csv:
>> frmt = CSVFormat(keys, options.skip_zero_records)
>> diff --git a/tools/kvm/kvm_stat/kvm_stat.txt b/tools/kvm/kvm_stat/kvm_stat.txt
>> index 24296dccc00a..de7c4a2497f9 100644
>> --- a/tools/kvm/kvm_stat/kvm_stat.txt
>> +++ b/tools/kvm/kvm_stat/kvm_stat.txt
>> @@ -92,6 +92,11 @@ OPTIONS
>> --log::
>> run in logging mode (like vmstat)
>>
>> +
>> +-L::
>> +--log-to-file::
>> + like '--log', but logging to a file
>
> -L<file>:: / --log-to-file=<file>
Argh...
Ciao,
Stefan
