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

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

[Fabiano Rosas]

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.

I'm open to suggestions on how to better implement this. One option
would be to ignore g_test_verbose() and have QTEST_LOG levels
(1,2,3,...), but before I get too deep into that, here are the raw
patches for discussion.

Note that it's not possible to use glib's g_test_trap_assert_stderr()
to silence the warnings because when using any verbose option the
output of QEMU, libqmp and qtest-log gets interleaved on stderr,
failing the match.

Thanks

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1442270209

Fabiano Rosas (2):
  tests/qtest: Mute QEMU stderr
  tests/qtest: Mute -qtest-log

 tests/qtest/libqtest.c              | 8 +++++---
 tests/qtest/test-x86-cpuid-compat.c | 6 ++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.35.3

[RFC PATCH 1/2] tests/qtest: Mute QEMU stderr

[Fabiano Rosas]

Make QEMU stderr conditional on the QTEST_LOG variable.

For the /x86/cpuid/parsing-plus-minus test, which traps the stderr, to
continue working set the QTEST_LOG variable from inside the
subprocess.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c              | 6 ++++--
 tests/qtest/test-x86-cpuid-compat.c | 6 ++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 1326e34291..347664cea6 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -476,11 +476,13 @@ static QTestState *qtest_init_internal(const char *qemu_bin,
                          "-display none "
                          "-audio none "
                          "%s"
-                         " -accel qtest",
+                         " -accel qtest"
+                         " 2> %s",
                          socket_path,
                          getenv("QTEST_LOG") ? DEV_STDERR : DEV_NULL,
                          qmp_socket_path,
-                         extra_args ?: "");
+                         extra_args ?: "",
+                         getenv("QTEST_LOG") ? DEV_STDERR : DEV_NULL);
 
     qtest_client_set_rx_handler(s, qtest_client_socket_recv_line);
     qtest_client_set_tx_handler(s, qtest_client_socket_send);
diff --git a/tests/qtest/test-x86-cpuid-compat.c b/tests/qtest/test-x86-cpuid-compat.c
index b9e7e5ef7b..641d1f8740 100644
--- a/tests/qtest/test-x86-cpuid-compat.c
+++ b/tests/qtest/test-x86-cpuid-compat.c
@@ -204,6 +204,9 @@ static void test_plus_minus_subprocess(void)
         return;
     }
 
+    const char *log = g_getenv("QTEST_LOG");
+    g_setenv("QTEST_LOG", "1", true);
+
     /* Rules:
      * 1)"-foo" overrides "+foo"
      * 2) "[+-]foo" overrides "foo=..."
@@ -227,6 +230,9 @@ static void test_plus_minus_subprocess(void)
     g_assert_true(qom_get_bool(path, "sse4-2"));
     g_assert_true(qom_get_bool(path, "sse4.2"));
 
+    if (log) {
+        g_setenv("QTEST_LOG", log, true);
+    }
     qtest_end();
     g_free(path);
 }
-- 
2.35.3

[RFC PATCH 2/2] tests/qtest: Mute -qtest-log

[Fabiano Rosas]

Make the -qtest-log be conditional on the --verbose flag, along with
the existing QTEST_LOG to avoid noisy qtest_memread() messages. Those
are particularly annoying for migration-test because all tests read
guest memory at the end and the QMP messages get lost in a flood of:

[R +1.096069] read 0x63ce000 0x1
[S +1.096071] OK 0xb8
[R +1.096077] read 0x63cf000 0x1
[S +1.096079] OK 0xb8
...

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 tests/qtest/libqtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 347664cea6..9fca9c7260 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -479,7 +479,7 @@ static QTestState *qtest_init_internal(const char *qemu_bin,
                          " -accel qtest"
                          " 2> %s",
                          socket_path,
-                         getenv("QTEST_LOG") ? DEV_STDERR : DEV_NULL,
+                         g_test_verbose() ? DEV_STDERR : DEV_NULL,
                          qmp_socket_path,
                          extra_args ?: "",
                          getenv("QTEST_LOG") ? DEV_STDERR : DEV_NULL);
-- 
2.35.3

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

[Thomas Huth]

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.

Not sure whether we want to ignore stderr by default... we might also miss 
important warnings or error messages that way...?

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.

  Thomas

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

[Daniel P. Berrangé]

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

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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

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

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

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

I could add error/warn variants that are noop in case qtest is
enabled. It would, however, lead to this pattern which is discouraged by
the error.h documentation (+Cc Markus for advice):

before:
    if (!dinfo && !qtest_enabled()) {
        error_report("A flash image must be given with the "
                     "'pflash' parameter");
        exit(1);
    }

after:
    if (!dinfo) {
        error_report_noqtest(&error_fatal,
                             "A flash image must be given with the "
                             "'pflash' parameter");
    }

For both error/warn, we'd reduce the amount of qtest_enabled() to only
the special cases not related to printing. We'd remove ~35/83 instances,
not counting the 7 printfs.

> 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

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

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> 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.
>>
>
> I could add error/warn variants that are noop in case qtest is
> enabled. It would, however, lead to this pattern which is discouraged by
> the error.h documentation (+Cc Markus for advice):
>
> before:
>     if (!dinfo && !qtest_enabled()) {
>         error_report("A flash image must be given with the "
>                      "'pflash' parameter");
>         exit(1);
>     }

This is connex_init() and verdex_init() in hw/arm/gumstix.c.

qtest_enabled() is *not* just suppressing a warning here, it's
suppressing a fatal error.  We use it to take a different codepath,
which is what Peter called out as a bad idea.

Comes from commit bdf921d65f8 (gumstix: Don't enforce use of -pflash for
qtest).

> after:
>     if (!dinfo) {
>         error_report_noqtest(&error_fatal,
>                              "A flash image must be given with the "
>                              "'pflash' parameter");
>     }

I don't like creating infrastructure to make bad ideas look less
obviously bad.

> For both error/warn, we'd reduce the amount of qtest_enabled() to only
> the special cases not related to printing. We'd remove ~35/83 instances,
> not counting the 7 printfs.
>
>> 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

Having tests provide the things users need to provide feels better.  It
may not always be practical.

I guess the example above is in this camp.

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

This is obviously fine, and if you guys want infrastructure for that,
I'll give it a sympathetic review.

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

This can be fine, but it's not obviously fine.

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

[Peter Maydell]

On Fri, 13 Sept 2024 at 11:02, Markus Armbruster <armbru@redhat.com> wrote:
>
> Fabiano Rosas <farosas@suse.de> writes:
> > I could add error/warn variants that are noop in case qtest is
> > enabled. It would, however, lead to this pattern which is discouraged by
> > the error.h documentation (+Cc Markus for advice):
> >
> > before:
> >     if (!dinfo && !qtest_enabled()) {
> >         error_report("A flash image must be given with the "
> >                      "'pflash' parameter");
> >         exit(1);
> >     }
>
> This is connex_init() and verdex_init() in hw/arm/gumstix.c.
>
> qtest_enabled() is *not* just suppressing a warning here, it's
> suppressing a fatal error.  We use it to take a different codepath,
> which is what Peter called out as a bad idea.
>
> Comes from commit bdf921d65f8 (gumstix: Don't enforce use of -pflash for
> qtest).

The good news on this one is that gumstix.c goes away in the
"arm: Drop deprecated boards" series, so this specific
error is moot :-) But it's in the same category as various
"-kernel is mandatory except with qtest" machine checks.

> > after:
> >     if (!dinfo) {
> >         error_report_noqtest(&error_fatal,
> >                              "A flash image must be given with the "
> >                              "'pflash' parameter");
> >     }
>
> I don't like creating infrastructure to make bad ideas look less
> obviously bad.
>
> > For both error/warn, we'd reduce the amount of qtest_enabled() to only
> > the special cases not related to printing. We'd remove ~35/83 instances,
> > not counting the 7 printfs.
> >
> >> 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
>
> Having tests provide the things users need to provide feels better.  It
> may not always be practical.

Specifically, if you don't disable the error-exit when qtest
is in use, then the generic qom-test tests which say "can we
at least instantiate every machine?" will fail, because they
assume that "qemu-system-foo -machine bar -accel qtest" will
at least start.

It doesn't really seem feasible to me to have qom-test
know about every machine's specific requirements for
how to pass a guest image.

The other approach would be to standardize on "every machine
type should happily start up with no warnings even if there
is no guest code specified by the user and it would simply
execute zeroes". We already do this for quite a lot of
boards, including some major ones, so we're certainly not
consistent about trying to diagnose user errors in this area.

thanks
-- PMM

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 13 Sept 2024 at 11:02, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Fabiano Rosas <farosas@suse.de> writes:
>> > I could add error/warn variants that are noop in case qtest is
>> > enabled. It would, however, lead to this pattern which is discouraged by
>> > the error.h documentation (+Cc Markus for advice):
>> >
>> > before:
>> >     if (!dinfo && !qtest_enabled()) {
>> >         error_report("A flash image must be given with the "
>> >                      "'pflash' parameter");
>> >         exit(1);
>> >     }
>>
>> This is connex_init() and verdex_init() in hw/arm/gumstix.c.
>>
>> qtest_enabled() is *not* just suppressing a warning here, it's
>> suppressing a fatal error.  We use it to take a different codepath,
>> which is what Peter called out as a bad idea.
>>
>> Comes from commit bdf921d65f8 (gumstix: Don't enforce use of -pflash for
>> qtest).
>
> The good news on this one is that gumstix.c goes away in the
> "arm: Drop deprecated boards" series, so this specific
> error is moot :-) But it's in the same category as various
> "-kernel is mandatory except with qtest" machine checks.
>
>> > after:
>> >     if (!dinfo) {
>> >         error_report_noqtest(&error_fatal,
>> >                              "A flash image must be given with the "
>> >                              "'pflash' parameter");
>> >     }
>>
>> I don't like creating infrastructure to make bad ideas look less
>> obviously bad.
>>
>> > For both error/warn, we'd reduce the amount of qtest_enabled() to only
>> > the special cases not related to printing. We'd remove ~35/83 instances,
>> > not counting the 7 printfs.
>> >
>> >> 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
>>
>> Having tests provide the things users need to provide feels better.  It
>> may not always be practical.
>
> Specifically, if you don't disable the error-exit when qtest
> is in use, then the generic qom-test tests which say "can we
> at least instantiate every machine?" will fail, because they
> assume that "qemu-system-foo -machine bar -accel qtest" will
> at least start.
>
> It doesn't really seem feasible to me to have qom-test
> know about every machine's specific requirements for
> how to pass a guest image.

Yes.

> The other approach would be to standardize on "every machine
> type should happily start up with no warnings even if there
> is no guest code specified by the user and it would simply
> execute zeroes". We already do this for quite a lot of
> boards, including some major ones, so we're certainly not
> consistent about trying to diagnose user errors in this area.

Fatal error unless qtest is bad, because we take a different path.

Silently executing zero can be hard for users to diagnose.

Possible compromise: warn unless qtest?

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

[Peter Maydell]

On Fri, 13 Sept 2024 at 12:29, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > Specifically, if you don't disable the error-exit when qtest
> > is in use, then the generic qom-test tests which say "can we
> > at least instantiate every machine?" will fail, because they
> > assume that "qemu-system-foo -machine bar -accel qtest" will
> > at least start.
> >
> > It doesn't really seem feasible to me to have qom-test
> > know about every machine's specific requirements for
> > how to pass a guest image.
>
> Yes.
>
> > The other approach would be to standardize on "every machine
> > type should happily start up with no warnings even if there
> > is no guest code specified by the user and it would simply
> > execute zeroes". We already do this for quite a lot of
> > boards, including some major ones, so we're certainly not
> > consistent about trying to diagnose user errors in this area.
>
> Fatal error unless qtest is bad, because we take a different path.
>
> Silently executing zero can be hard for users to diagnose.
>
> Possible compromise: warn unless qtest?

That runs into the "tests that pass and do what they're
supposed to do shouldn't provoke warnings" unofficial
guideline... Some of these qtest_enabled() checks are
exactly to suppress a warning.

-- PMM

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

[Thomas Huth]

On 13/09/2024 13.46, Peter Maydell wrote:
> On Fri, 13 Sept 2024 at 12:29, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> Specifically, if you don't disable the error-exit when qtest
>>> is in use, then the generic qom-test tests which say "can we
>>> at least instantiate every machine?" will fail, because they
>>> assume that "qemu-system-foo -machine bar -accel qtest" will
>>> at least start.
>>>
>>> It doesn't really seem feasible to me to have qom-test
>>> know about every machine's specific requirements for
>>> how to pass a guest image.
>>
>> Yes.
>>
>>> The other approach would be to standardize on "every machine
>>> type should happily start up with no warnings even if there
>>> is no guest code specified by the user and it would simply
>>> execute zeroes". We already do this for quite a lot of
>>> boards, including some major ones, so we're certainly not
>>> consistent about trying to diagnose user errors in this area.

IMHO executing zeros is also a bad idea ... most of those boards crash after 
a while when the program counter reaches an unmapped memory region.

Maybe we could simply put a "branch to self" instruction on the first 
program counter address in case the kernel/firmware cannot be loaded?

>> Fatal error unless qtest is bad, because we take a different path.
>>
>> Silently executing zero can be hard for users to diagnose.
>>
>> Possible compromise: warn unless qtest?
> 
> That runs into the "tests that pass and do what they're
> supposed to do shouldn't provoke warnings" unofficial
> guideline... Some of these qtest_enabled() checks are
> exactly to suppress a warning.

FWIW, I like the idea of having a error_report_user() function that is 
silent when running with qtest_enabled().

  Thomas

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

[Daniel P. Berrangé]

On Fri, Sep 06, 2024 at 10:52:53AM +0100, Peter Maydell wrote:
> 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.

Currently we just allow the child QEMU process stdout/err
to inherit to the qtest program's stdout/err. With that
approach we have to do filtering at soruce (ie in QEMU
itself). I feel that in general it is a bad idea for the
program being tested to alter itself to suit the test
suite, not least because two different parts of the test
suite may have differing views about what messages they
want to ignore vs display.

We could address this be switching to the model used
with IO tests.  Always capture the child QEMU process
stdout/err to a pipe. The test program can apply regex
filters to cull output that is expected & irrelevant,
and then print out whatever is left over on its own
stderr.

That way all the filtering of undesirable messages would
be exclusively in the test suite, not QEMU itself.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Sep 06, 2024 at 10:52:53AM +0100, Peter Maydell wrote:
>> 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.
>
> Currently we just allow the child QEMU process stdout/err
> to inherit to the qtest program's stdout/err. With that
> approach we have to do filtering at soruce (ie in QEMU
> itself). I feel that in general it is a bad idea for the
> program being tested to alter itself to suit the test
> suite, not least because two different parts of the test
> suite may have differing views about what messages they
> want to ignore vs display.
>
> We could address this be switching to the model used
> with IO tests.  Always capture the child QEMU process
> stdout/err to a pipe. The test program can apply regex
> filters to cull output that is expected & irrelevant,
> and then print out whatever is left over on its own
> stderr.
>
> That way all the filtering of undesirable messages would
> be exclusively in the test suite, not QEMU itself.

Point.