diff mbox series

[v4,17/32] system/qtest: properly feedback results of clock_[step|set]

Message ID 20250108121054.1126164-18-alex.bennee@linaro.org
State Superseded
Headers show
Series testing/next: functional tests, qtest clocks, vm and keymaps (pre-PR-FINAL) | expand

Commit Message

Alex Bennée Jan. 8, 2025, 12:10 p.m. UTC 
Time will not advance if the system is paused or there are no timer
events set for the future. In absence of pending timer events
advancing time would make no difference the system state. Attempting
to do so would be a bug and the test or device under test would need
fixing.

Tighten up the result reporting to `FAIL` if time was not advanced.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2687
---
 system/qtest.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Michael Tokarev Jan. 13, 2025, 3:22 p.m. UTC | #1 
Ghrm. 46 recipients seems to be quite a bit too aggressive..

08.01.2025 15:10, Alex Bennée wrote:
> Time will not advance if the system is paused or there are no timer
> events set for the future. In absence of pending timer events
> advancing time would make no difference the system state. Attempting
> to do so would be a bug and the test or device under test would need
> fixing.
> 
> Tighten up the result reporting to `FAIL` if time was not advanced.

Is this qemu-stable material?

Thanks,

/mjt
Alex Bennée Jan. 13, 2025, 4:26 p.m. UTC | #2 
Michael Tokarev <mjt@tls.msk.ru> writes:

> Ghrm. 46 recipients seems to be quite a bit too aggressive..

I think git-publish just accumulates Cc's from each run for a given
branch. While I can reset the version counter I'm not sure where I can
reset the Cc list from.

> 08.01.2025 15:10, Alex Bennée wrote:
>> Time will not advance if the system is paused or there are no timer
>> events set for the future. In absence of pending timer events
>> advancing time would make no difference the system state. Attempting
>> to do so would be a bug and the test or device under test would need
>> fixing.
>> Tighten up the result reporting to `FAIL` if time was not advanced.
>
> Is this qemu-stable material?

Probably not - I guess that was a Cc from a previous series that stuck?

>
> Thanks,
>
> /mjt
Michael Tokarev Jan. 13, 2025, 5:52 p.m. UTC | #3 
13.01.2025 19:26, Alex Bennée wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> Ghrm. 46 recipients seems to be quite a bit too aggressive..
> 
> I think git-publish just accumulates Cc's from each run for a given
> branch. While I can reset the version counter I'm not sure where I can
> reset the Cc list from.

I never used git-publish.  It's fun to know which other tools do people
use :)

>> 08.01.2025 15:10, Alex Bennée wrote:
>>> Time will not advance if the system is paused or there are no timer
>>> events set for the future. In absence of pending timer events
>>> advancing time would make no difference the system state. Attempting
>>> to do so would be a bug and the test or device under test would need
>>> fixing.
>>> Tighten up the result reporting to `FAIL` if time was not advanced.
>>
>> Is this qemu-stable material?
> 
> Probably not - I guess that was a Cc from a previous series that stuck?

No, it's just that it fixes an issue reported on gitlab (!2687),
which apparently weren't fixed in time for 9.2, - that's why :)

Thanks,

/mjt
diff mbox series

Patch

diff --git a/system/qtest.c b/system/qtest.c
index 99ef2042f6..e68ed0f2a8 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -78,6 +78,11 @@  static void *qtest_server_send_opaque;
  * let you adjust the value of the clock (monotonically).  All the commands
  * return the current value of the clock in nanoseconds.
  *
+ * If the commands FAIL then time wasn't advanced which is likely
+ * because the machine was in a paused state or no timer events exist
+ * in the future. This will cause qtest to abort and the test will
+ * need to check its assumptions.
+ *
  * .. code-block:: none
  *
  *  > clock_step
@@ -710,7 +715,8 @@  static void qtest_process_command(CharBackend *chr, gchar **words)
             qtest_sendf(chr, "OK little\n");
         }
     } else if (qtest_enabled() && strcmp(words[0], "clock_step") == 0) {
-        int64_t ns;
+        int64_t old_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        int64_t ns, new_ns;
 
         if (words[1]) {
             int ret = qemu_strtoi64(words[1], NULL, 0, &ns);
@@ -719,11 +725,10 @@  static void qtest_process_command(CharBackend *chr, gchar **words)
             ns = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
                                             QEMU_TIMER_ATTR_ALL);
         }
-        qemu_clock_advance_virtual_time(
-            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + ns);
+        new_ns = qemu_clock_advance_virtual_time(old_ns + ns);
         qtest_send_prefix(chr);
-        qtest_sendf(chr, "OK %"PRIi64"\n",
-                    (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        qtest_sendf(chr, "%s %"PRIi64"\n",
+                    new_ns > old_ns ? "OK" : "FAIL", new_ns);
     } else if (strcmp(words[0], "module_load") == 0) {
         Error *local_err = NULL;
         int rv;
@@ -740,16 +745,16 @@  static void qtest_process_command(CharBackend *chr, gchar **words)
             qtest_sendf(chr, "FAIL\n");
         }
     } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) {
-        int64_t ns;
+        int64_t ns, new_ns;
         int ret;
 
         g_assert(words[1]);
         ret = qemu_strtoi64(words[1], NULL, 0, &ns);
         g_assert(ret == 0);
-        qemu_clock_advance_virtual_time(ns);
+        new_ns = qemu_clock_advance_virtual_time(ns);
         qtest_send_prefix(chr);
-        qtest_sendf(chr, "OK %"PRIi64"\n",
-                    (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        qtest_sendf(chr, "%s %"PRIi64"\n",
+                    new_ns == ns ? "OK" : "FAIL", new_ns);
     } else if (process_command_cb && process_command_cb(chr, words)) {
         /* Command got consumed by the callback handler */
     } else {