diff mbox series

[18/20] system/qtest: properly feedback results of clock_[step|set]

Message ID 20241210204349.723590-19-alex.bennee@linaro.org
State Superseded
Headers show
Series testing/next: functional tests and qtest timers | expand

Commit Message

Alex Bennée Dec. 10, 2024, 8:43 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>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2687
---
 system/qtest.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Thomas Huth Dec. 11, 2024, 6:35 a.m. UTC | #1 
On 10/12/2024 21.43, 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.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2687
> ---
>   system/qtest.c | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/system/qtest.c b/system/qtest.c
> index 12703a2045..d9501153a4 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 {

Reviewed-by: Thomas Huth <thuth@redhat.com>
Fabiano Rosas Dec. 12, 2024, 12:36 p.m. UTC | #2 
Alex Bennée <alex.bennee@linaro.org> writes:

> 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>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2687

Acked-by: Fabiano Rosas <farosas@suse.de>
diff mbox series

Patch

diff --git a/system/qtest.c b/system/qtest.c
index 12703a2045..d9501153a4 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 {