Re: [Qemu-devel] [PATCH v4 17/22] libqtest: Add qmp_args() helper

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Similar to the previous helper, we can reduce the boilerplate
> of most callers by passing the command name separately from
> the interpolated arguments.  Adjust the majority of the callers
> that can use the new helpers; in the process, fixing a few
> places where we would have failed -Wformat-nonliteral.  The
> new helper function already uses GCC_FMT_ATTR to match the
> underlying use of qobject_from_jsonv().
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  tests/libqtest.h               |  20 ++++++
>  tests/libqtest.c               |  29 +++++++++
>  tests/ahci-test.c              |  19 +++---
>  tests/device-introspect-test.c |   4 +-
>  tests/drive_del-test.c         |  10 +--
>  tests/fdc-test.c               |   8 +--
>  tests/libqos/libqos.c          |   7 +--
>  tests/libqos/pci-pc.c          |   8 +--
>  tests/pc-cpu-test.c            |   8 +--
>  tests/postcopy-test.c          |  30 +++------
>  tests/qom-test.c               |   9 +--
>  tests/test-netfilter.c         | 139 
> ++++++++++++++++-------------------------
>  tests/test-x86-cpuid-compat.c  |   6 +-
>  tests/tmp105-test.c            |   9 +--
>  tests/usb-hcd-uhci-test.c      |  14 ++---
>  tests/usb-hcd-xhci-test.c      |  25 ++------
>  tests/vhost-user-test.c        |  12 +---
>  tests/virtio-blk-test.c        |  12 +---
>  tests/virtio-scsi-test.c       |  13 +---
>  tests/virtio-serial-test.c     |  12 +---
>  20 files changed, 167 insertions(+), 227 deletions(-)
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index e0d87d035a..86ca7fa581 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -499,6 +499,26 @@ QDict *qmp_cmd(const char *cmd);
>  void qmp_cmd_async(const char *cmd);
>
>  /**
> + * qmp_args:
> + * @cmd: QMP command to send to QEMU.
> + * @fmt...: Arguments for the command; formats arguments through
> + * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf%])').
> + *
> + * Sends a QMP message to QEMU and returns the response.
> + */
> +QDict *qmp_args(const char *cmd, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
> +
> +/**
> + * qmp_args_async:
> + * @cmd: QMP command to send to QEMU.
> + * @fmt...: Arguments for the command; formats arguments through
> + * json-lexer.c (only understands '%(PRI[ud]64|(l|ll)?[du]|[ipsf%])').
> + *
> + * Sends a QMP message to QEMU and leaves the response in the stream.
> + */
> +void qmp_args_async(const char *cmd, const char *fmt, ...) GCC_FMT_ATTR(2, 
> 3);
> +
> +/**
>   * qmp_discard_response:
>   *
>   * Read and discard a QMP response, typically after qmp_async().
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 3926a4d481..49786cf2d7 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -869,6 +869,35 @@ void qmp_cmd_async(const char *cmd)
>      qtest_qmp_send(global_qtest, "{'execute':%s}", cmd);
>  }
>
> +static void qmp_args_dict_async(const char *cmd, QDict *args)
> +{
> +    assert(args);
> +    qtest_qmp_send(global_qtest, "{'execute':%s, 'arguments':%p}", cmd, 
> args);
> +}
> +
> +QDict *qmp_args(const char *cmd, const char *fmt, ...)
> +{
> +    va_list ap;
> +    QObject *obj;
> +
> +    va_start(ap, fmt);
> +    obj = qobject_from_jsonv(fmt, ap);
> +    va_end(ap);
> +    qmp_args_dict_async(cmd, qobject_to_qdict(obj));
> +    return qtest_qmp_receive(global_qtest);
> +}
> +
> +void qmp_args_async(const char *cmd, const char *fmt, ...)
> +{
> +    va_list ap;
> +    QObject *obj;
> +
> +    va_start(ap, fmt);
> +    obj = qobject_from_jsonv(fmt, ap);
> +    va_end(ap);
> +    qmp_args_dict_async(cmd, qobject_to_qdict(obj));
> +}
> +

I don't like the qmp_args name.  It's not about arguments, it's about
sending a command with arguments.

I'm afraid I'm getting pretty thorougly confused by the evolving
interface.  So I stop and think how it should look like when done.

We need send and receive.  Convenience functions to do both, and to
receive and throw away are nice to have.

We need qmp_raw().  We haven't found a need for a raw receive function.

Convenience functions to build commands and send are nice to have.  They
come in pairs, without arguments, to avoid -Wno-format-zero-length
(aside: that's one of the sillier warnings).

We used to have almost everything with and without QTestState, but we're
refusing to continue that self-flagellation.

For every function taking ..., we want one taking va_list.

Makes sense?

Can we derive a sensible set of function names from this?

[...]