Re: [PATCH kvmtool v2 2/4] Replace printf/fprintf with pr_* macros

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Suzuki,

On Wed, Jul 12, 2023 at 05:26:24PM +0100, Suzuki K Poulose wrote:
> Hi Alexandru
> 
> On 07/07/2023 16:11, Alexandru Elisei wrote:
> > To prepare for allowing finer control over the messages that kvmtool
> > displays, replace printf() and fprintf() with the pr_* macros.
> > 
> > Minor changes were made to fix coding style issues that were pet peeves for
> > the author. And use pr_err() in kvm_cpu__init() instead of pr_warning() for
> > fatal errors.
> > 
> > Also, fix the message when printing the exit code for KVM_EXIT_UNKNOWN by
> > removing the '0x' part, because it's printing a decimal number, not a
> > hexadecimal one (the format specifier is %llu, not %llx).
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> > Changelog:
> > 
> > - Use pr_err() to directly replace fprintf() in kernel_usage_with_options()
> >    instead of concatening the kernel locations.
> > - Removed the '0x' from the "KVM exit code: 0x%llu" message in kvm_cpu_thread()
> >    because the number is decimal (it's %llu, not %llx).
> > - Reverted the changes to kvm__emulate_mmio() and debug_io () because those
> >    messages are displayed with --debug-mmio, respectively --debug-ioport, and
> >    --loglevel hiding them would have been counter-intuitive.
> > - Replaced the "warning" string in kvm__emulate_mmio() with "MMIO warning", to
> >    match the message from kvm__emulate_io(). And to make it clear that it isn't
> >    toggled with --loglevel.
> > - Removed extra spaces in virtio_compat_add_message().
> > 
> >   arm/gic.c       |  5 ++---
> >   builtin-run.c   | 37 +++++++++++++++++++------------------
> >   builtin-setup.c | 16 ++++++++--------
> >   guest_compat.c  |  2 +-
> >   kvm-cpu.c       | 12 ++++++------
> >   mmio.c          |  2 +-
> >   6 files changed, 37 insertions(+), 37 deletions(-)
> > 
> 
> > diff --git a/guest_compat.c b/guest_compat.c
> > index fd4704b20b16..93f9aabcd6db 100644
> > --- a/guest_compat.c
> > +++ b/guest_compat.c
> > @@ -86,7 +86,7 @@ int compat__print_all_messages(void)
> >   		msg = list_first_entry(&messages, struct compat_message, list);
> > -		printf("\n  # KVM compatibility warning.\n\t%s\n\t%s\n",
> > +		pr_warning("KVM compatibility warning.\n\t%s\n\t%s",
> >   			msg->title, msg->desc);
> 
> Does this really need to be a Warning ? A user could be running a non-
> Linux guest and reporting the compatibility with WARNING level makes it
> a bit tricky to suppress. i.e., User may want to suppress the "virtio"
> compatibility messages, without actually loosing any other important
> "Warnings". With the --loglevel=warning, we don't have that capability.

That sounds reasonable.

> 
> There are two options here as far as I can see:
> 
> 1) Convert compatibility messages to "Info"
> 2) Control the compatibility messages via new option, (something like we
>    did here, with --nocompat [0])

Out of the two, I would rather go the "info" route, so there aren't two
separate methods to disable kvmtool messages, via --loglevel and via
--nocompat.

I'll prepare a patch.

Thanks,
Alex

> 
> [0]
> https://lore.kernel.org/all/20230127113932.166089-5-suzuki.poulose@arm.com/
> 
> 
> Suzuki
>