diff mbox series

[6/7] tests/qtest: tighten up the checks on clock_step

Message ID 20250120210212.3890255-7-alex.bennee@linaro.org
State New
Headers show
Series testing/next (qtest timer stuff) | expand

Commit Message

Alex Bennée Jan. 20, 2025, 9:02 p.m. UTC 
It is invalid to call clock_step with an implied time to step forward
as if no timers are running we won't be able to advance.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 system/qtest.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Peter Maydell Jan. 21, 2025, 1:04 p.m. UTC | #1 
On Mon, 20 Jan 2025 at 21:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> It is invalid to call clock_step with an implied time to step forward
> as if no timers are running we won't be able to advance.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  system/qtest.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/system/qtest.c b/system/qtest.c
> index 28b6fac37c..1a9bfd0b33 100644
> --- a/system/qtest.c
> +++ b/system/qtest.c
> @@ -708,10 +708,15 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
>          } else {
>              ns = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
>                                              QEMU_TIMER_ATTR_ALL);
> +            if (ns < 0) {
> +                qtest_send(chr, "FAIL "
> +                           "no timers for clock_step to follow\n");

I think I would say
"cannot advance clock to the next deadline because there is no pending deadline"
as being a bit clearer about what's gone wrong here.

> +                return;
> +            }
>          }
>          new_ns = qemu_clock_advance_virtual_time(old_ns + ns);
>          qtest_sendf(chr, "%s %"PRIi64"\n",
> -                    new_ns > old_ns ? "OK" : "FAIL", new_ns);
> +                    new_ns > old_ns ? "OK" : "FAIL could not advance time", new_ns);

Maybe we should give up on trying to handle the OK and FAIL
cases in the same qtest_sendf() call? It's not clear to me that
printing the new clock value in the FAIL message is actually
useful.

For that matter, is it actually possible for the clock to not
advance? It doesn't seem obvious that "advance the clock by 0 ticks"
should be a failure case rather than a "does nothing" case,
and once patch 7 is applied I don't think there's any case
where qemu_clock_advance_virtual_time() could make the
clock go backwards... Put another way, this would be moving
back to the situation before your commit d524441a3610b.

thanks
-- PMM
diff mbox series

Patch

diff --git a/system/qtest.c b/system/qtest.c
index 28b6fac37c..1a9bfd0b33 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -708,10 +708,15 @@  static void qtest_process_command(CharBackend *chr, gchar **words)
         } else {
             ns = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
                                             QEMU_TIMER_ATTR_ALL);
+            if (ns < 0) {
+                qtest_send(chr, "FAIL "
+                           "no timers for clock_step to follow\n");
+                return;
+            }
         }
         new_ns = qemu_clock_advance_virtual_time(old_ns + ns);
         qtest_sendf(chr, "%s %"PRIi64"\n",
-                    new_ns > old_ns ? "OK" : "FAIL", new_ns);
+                    new_ns > old_ns ? "OK" : "FAIL could not advance time", new_ns);
     } else if (strcmp(words[0], "module_load") == 0) {
         Error *local_err = NULL;
         int rv;