Re: [RFC PATCH 0/2] qtest: Log verbosity changes

[Peter Maydell]

On Fri, 6 Sept 2024 at 09:14, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, Sep 06, 2024 at 08:16:31AM +0200, Thomas Huth wrote:
> > On 05/09/2024 23.03, Fabiano Rosas wrote:
> > > Hi,
> > >
> > > This series silences QEMU stderr unless the QTEST_LOG variable is set
> > > and silences -qtest-log unless both QTEST_LOG and gtest's --verbose
> > > flag is passed.
> > >
> > > This was motivated by Peter Maydell's ask to suppress deprecation
> > > warn_report messages from the migration-tests and by my own
> > > frustration over noisy output from qtest.

This isn't what I want, though -- what I want is that a
qtest run should not print "warning:" messages for things
that we expect to happen when we run that test. I *do* want
warnings for things that we do not expect to happen when
we run the test.

> > Not sure whether we want to ignore stderr by default... we might also miss
> > important warnings or error messages that way...?
>
> I would prefer if our tests were quiet by default, and just printed
> clear pass/fail notices without extraneous fluff. Having an opt-in
> to see full messages from stderr feels good enough for debugging cases
> where you need more info from a particular test.

I find it is not uncommon that something fails and
you don't necessarily have the option to re-run it with
the "give me the error message this time" flag turn on.
CI is just the most obvious example; other kinds of
intermittent failure can be similar.

> Probably we should set verbose mode in CI though, since it is tedious
> to re-run CI on failure to gather more info
>
> > If you just want to suppress one certain warning, I think it's maybe best to
> > fence it with "if (!qtest_enabled()) { ... }" on the QEMU side - at least
> > that's what we did in similar cases a couple of times, IIRC.
>
> We're got a surprisingly large mumber of if(qtest_enabled()) conditions
> in the code. I can't help feeling this is a bad idea in the long term,
> as its making us take different codepaths when testing from production.

What I want from CI and from tests in general:
 * if something fails, I want all the information
 * if something unexpected happens I want the warning even
   if the test passes (this is why I grep the logs for
   "warning:" in the first place -- it is to catch the case
   of "something went wrong but it didn't result in QEMU or
   the test case exiting with a failure status")
 * if something is expected, it should be silent

That means there's a class of messages where we want to warn
the user that they're doing something that might not be what
they intended or which is deprecated and will go away soon,
but where we do not want to "warn" in the test logging because
the test is deliberately setting up that oddball corner case.

It might be useful to have a look at where we're using
if (qtest_enabled()) to see if we can make some subcategories
avoid the explicit if(), e.g. by having a warn_deprecated(...)
and hide the "don't print if qtest" inside the function.

Some categories as a starter:
 * some board models will error-and-exit if the user
   didn't provide any guest code (eg no -kernel option),
   like hw/m68k/an5206.c. When we're running with the
   qtest accelerator it's fine and expected that there's
   no guest code loaded because we'll never run any guest code
 * in some places (eg target/arm/cpu.c) we treat qtest as
   another accelerator type, so we might say
   if (tcg_enabled() || qtest_enabled()) to mean "not
   hvf or kvm"
 * sometimes we print a deprecation message only if
   not qtest, eg hw/core/numa.c or hw/core/machine.c
 * the clock related code needs to be qtest aware because
   under qtest it's the qtest protocol that advances the
   clock
 * sometimes we warn about possible user error if not
   qtest, eg hw/ppc/pnv.c or target/mips/cpu.c

thanks
-- PMM