Re: [Qemu-devel] [PATCH v4 04/22] tests: Add assertion for no qmp("")

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Now that the previous patches have fixed all callers to avoid an
> empty message, we can tweak qmp_fd_sendv() to assert that we
> don't introduce new callers, and reindent accordingly.  The
> additional assertions will also help verify that later refactoring
> is not breaking anything.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/libqtest.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 7e5425d704..99a07c246f 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -450,6 +450,9 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>  {
>      va_list ap_copy;
>      QObject *qobj;
> +    int log = getenv("QTEST_LOG") != NULL;

Use the opportunity to make this bool?

> +    QString *qstr;
> +    const char *str;
>
>      /* qobject_from_jsonv() silently eats leading 0xff as invalid
>       * JSON, but we want to test sending them over the wire to force
> @@ -458,6 +461,7 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>          socket_send(fd, fmt, 1);
>          fmt++;
>      }
> +    assert(*fmt);

I prefer assertions on arguments going first, for extra visibility.

>      /* Going through qobject ensures we escape strings properly.
>       * This seemingly unnecessary copy is required in case va_list
> @@ -466,29 +470,23 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>      va_copy(ap_copy, ap);
>      qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
>      va_end(ap_copy);
> +    qstr = qobject_to_json(qobj);
>
> -    /* No need to send anything for an empty QObject.  */
> -    if (qobj) {
> -        int log = getenv("QTEST_LOG") != NULL;
> -        QString *qstr = qobject_to_json(qobj);
> -        const char *str;
> +    /*
> +     * BUG: QMP doesn't react to input until it sees a newline, an
> +     * object, or an array.  Work-around: give it a newline.
> +     */
> +    qstring_append_chr(qstr, '\n');
> +    str = qstring_get_str(qstr);
>
> -        /*
> -         * BUG: QMP doesn't react to input until it sees a newline, an
> -         * object, or an array.  Work-around: give it a newline.
> -         */
> -        qstring_append_chr(qstr, '\n');
> -        str = qstring_get_str(qstr);
> -
> -        if (log) {
> -            fprintf(stderr, "%s", str);
> -        }
> -        /* Send QMP request */
> -        socket_send(fd, str, qstring_get_length(qstr));
> -
> -        QDECREF(qstr);
> -        qobject_decref(qobj);
> +    if (log) {
> +        fprintf(stderr, "%s", str);
>      }
> +    /* Send QMP request */
> +    socket_send(fd, str, qstring_get_length(qstr));
> +
> +    QDECREF(qstr);
> +    qobject_decref(qobj);
>  }
>
>  void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap)

Preferably with the assertion moved:
Reviewed-by: Markus Armbruster <armbru@redhat.com>