Re: [PATCH] libqtest: Rework qtest_rsp()

[Marc-André Lureau]

On Tue, Jan 26, 2021 at 7:19 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> qtest_rsp() is used in two different ways: (1) return some arguments
> to caller, which the caller must free, and (2) return no arguments to
> caller.  Passing non-zero @expected_args gets you (1), and passing
> zero gets you (2).
>
> Having "the return value must be freed" depend on an argument this way
> is less than ideal.
>
> Provide separate functions for the two ways: (1) qtest_rsp_args()
> takes @expected_args (possibly zero), and returns that number of
> arguments.  Caller must free the return value always.  (2) qtest_rsp()
> assumes zero, and returns nothing.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qtest/libqtest.c | 50 ++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 5249a628cc..fd043b0570 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -503,7 +503,7 @@ static GString *qtest_client_socket_recv_line(QTestState 
> *s)
>      return line;
>  }
>
> -static gchar **qtest_rsp(QTestState *s, int expected_args)
> +static gchar **qtest_rsp_args(QTestState *s, int expected_args)
>  {
>      GString *line;
>      gchar **words;
> @@ -539,25 +539,27 @@ redo:
>      g_assert(words[0] != NULL);
>      g_assert_cmpstr(words[0], ==, "OK");
>
> -    if (expected_args) {
> -        for (i = 0; i < expected_args; i++) {
> -            g_assert(words[i] != NULL);
> -        }
> -    } else {
> -        g_strfreev(words);
> -        words = NULL;
> +    for (i = 0; i < expected_args; i++) {
> +        g_assert(words[i] != NULL);
>      }
>
>      return words;
>  }
>
> +static void qtest_rsp(QTestState *s)
> +{
> +    gchar **words = qtest_rsp_args(s, 0);

You may even use g_auto(GStrv) if you fancy that.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> +
> +    g_strfreev(words);
> +}
> +
>  static int qtest_query_target_endianness(QTestState *s)
>  {
>      gchar **args;
>      int big_endian;
>
>      qtest_sendf(s, "endianness\n");
> -    args = qtest_rsp(s, 1);
> +    args = qtest_rsp_args(s, 1);
>      g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
>      big_endian = strcmp(args[1], "big") == 0;
>      g_strfreev(args);
> @@ -892,14 +894,14 @@ bool qtest_get_irq(QTestState *s, int num)
>  void qtest_module_load(QTestState *s, const char *prefix, const char 
> *libname)
>  {
>      qtest_sendf(s, "module_load %s %s\n", prefix, libname);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>  }
>
>  static int64_t qtest_clock_rsp(QTestState *s)
>  {
>      gchar **words;
>      int64_t clock;
> -    words = qtest_rsp(s, 2);
> +    words = qtest_rsp_args(s, 2);
>      clock = g_ascii_strtoll(words[1], NULL, 0);
>      g_strfreev(words);
>      return clock;
> @@ -926,13 +928,13 @@ int64_t qtest_clock_set(QTestState *s, int64_t val)
>  void qtest_irq_intercept_out(QTestState *s, const char *qom_path)
>  {
>      qtest_sendf(s, "irq_intercept_out %s\n", qom_path);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>  }
>
>  void qtest_irq_intercept_in(QTestState *s, const char *qom_path)
>  {
>      qtest_sendf(s, "irq_intercept_in %s\n", qom_path);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>  }
>
>  void qtest_set_irq_in(QTestState *s, const char *qom_path, const char *name,
> @@ -942,13 +944,13 @@ void qtest_set_irq_in(QTestState *s, const char 
> *qom_path, const char *name,
>          name = "unnamed-gpio-in";
>      }
>      qtest_sendf(s, "set_irq_in %s %s %d %d\n", qom_path, name, num, level);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>  }
>
>  static void qtest_out(QTestState *s, const char *cmd, uint16_t addr, 
> uint32_t value)
>  {
>      qtest_sendf(s, "%s 0x%x 0x%x\n", cmd, addr, value);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>  }
>
>  void qtest_outb(QTestState *s, uint16_t addr, uint8_t value)
> @@ -973,7 +975,7 @@ static uint32_t qtest_in(QTestState *s, const char *cmd, 
> uint16_t addr)
>      unsigned long value;
>
>      qtest_sendf(s, "%s 0x%x\n", cmd, addr);
> -    args = qtest_rsp(s, 2);
> +    args = qtest_rsp_args(s, 2);
>      ret = qemu_strtoul(args[1], NULL, 0, &value);
>      g_assert(!ret && value <= UINT32_MAX);
>      g_strfreev(args);
> @@ -1000,7 +1002,7 @@ static void qtest_write(QTestState *s, const char *cmd, 
> uint64_t addr,
>                          uint64_t value)
>  {
>      qtest_sendf(s, "%s 0x%" PRIx64 " 0x%" PRIx64 "\n", cmd, addr, value);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>  }
>
>  void qtest_writeb(QTestState *s, uint64_t addr, uint8_t value)
> @@ -1030,7 +1032,7 @@ static uint64_t qtest_read(QTestState *s, const char 
> *cmd, uint64_t addr)
>      uint64_t value;
>
>      qtest_sendf(s, "%s 0x%" PRIx64 "\n", cmd, addr);
> -    args = qtest_rsp(s, 2);
> +    args = qtest_rsp_args(s, 2);
>      ret = qemu_strtou64(args[1], NULL, 0, &value);
>      g_assert(!ret);
>      g_strfreev(args);
> @@ -1082,7 +1084,7 @@ void qtest_memread(QTestState *s, uint64_t addr, void 
> *data, size_t size)
>      }
>
>      qtest_sendf(s, "read 0x%" PRIx64 " 0x%zx\n", addr, size);
> -    args = qtest_rsp(s, 2);
> +    args = qtest_rsp_args(s, 2);
>
>      for (i = 0; i < size; i++) {
>          ptr[i] = hex2nib(args[1][2 + (i * 2)]) << 4;
> @@ -1098,7 +1100,7 @@ uint64_t qtest_rtas_call(QTestState *s, const char 
> *name,
>  {
>      qtest_sendf(s, "rtas %s %u 0x%"PRIx64" %u 0x%"PRIx64"\n",
>                  name, nargs, args, nret, ret);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>      return 0;
>  }
>
> @@ -1134,7 +1136,7 @@ void qtest_bufwrite(QTestState *s, uint64_t addr, const 
> void *data, size_t size)
>      qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
>      s->ops.send(s, bdata);
>      s->ops.send(s, "\n");
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>      g_free(bdata);
>  }
>
> @@ -1144,7 +1146,7 @@ void qtest_bufread(QTestState *s, uint64_t addr, void 
> *data, size_t size)
>      size_t len;
>
>      qtest_sendf(s, "b64read 0x%" PRIx64 " 0x%zx\n", addr, size);
> -    args = qtest_rsp(s, 2);
> +    args = qtest_rsp_args(s, 2);
>
>      g_base64_decode_inplace(args[1], &len);
>      if (size != len) {
> @@ -1174,14 +1176,14 @@ void qtest_memwrite(QTestState *s, uint64_t addr, 
> const void *data, size_t size)
>      }
>
>      qtest_sendf(s, "write 0x%" PRIx64 " 0x%zx 0x%s\n", addr, size, enc);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>      g_free(enc);
>  }
>
>  void qtest_memset(QTestState *s, uint64_t addr, uint8_t pattern, size_t size)
>  {
>      qtest_sendf(s, "memset 0x%" PRIx64 " 0x%zx 0x%02x\n", addr, size, 
> pattern);
> -    qtest_rsp(s, 0);
> +    qtest_rsp(s);
>  }
>
>  void qtest_qmp_assert_success(QTestState *qts, const char *fmt, ...)
> --
> 2.26.2
>
>


-- 
Marc-André Lureau