Re: [Qemu-devel] [PATCH v4 12/22] libqtest: Change qmp_fd_send() to drop varargs

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> With the previous commit, no external clients are using qmp_fd()
> or qmp_fd_sendv().  Making qmp_fd_sendv() static lets us refactor
> the public qmp_fd_send() to be the common point where we send a
> fixed string over the wire as well as log that string, while
> qmp_fd_sendv() worries about converting varargs into the final
> string.  Note that the refactoring changes roles: previously,
> qmp_fd_send() deferred to qmp_fd_sendv(); now the call chain is
> in the opposite direction.  Likewise, now that we take a fixed
> string, we no longer have to special case '\xff'.

I'm fine with the reversal of roles.  The name qmp_fd_send() becomes
slightly problematic, though: it's *not* the ... variant of
qmp_fd_sendv(), as is the case for similar pairs of function names.

I wish libqtest's naming would follow libc conventions more closely.
libc: printf() and vprintf(), sprintf() and vsprintf().  libqtest:
qmp_fd_send() and qmp_sendv(), qtest_hmp() and qtest_hmpv(), ...
Exceptions (sort of): socket_send() and socket_sendf(), qtest_sendf().
Not sure this is worth cleaning up now.

If we decice it is, then the name qmp_fd_send() would be fine, because
its va_list buddy would be qmp_fd_vsendf(), and its ... buddy would be
qmp_fd_sendf().

> Add documentation while in the area.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  tests/libqtest.h | 20 ++++++++++++----
>  tests/libqtest.c | 73 
> +++++++++++++++++---------------------------------------
>  tests/test-qga.c |  2 --
>  3 files changed, 38 insertions(+), 57 deletions(-)
>
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 38148af66b..33af3ae8ff 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -923,11 +923,23 @@ static inline int64_t clock_set(int64_t val)
>      return qtest_clock_set(global_qtest, val);
>  }
>
> +/**
> + * qmp_fd_receive:
> + * @fd: Socket to read from.
> + *
> + * Read from input until a complete JSON object has been parsed,
> + * returning NULL on errors.
> + */
>  QDict *qmp_fd_receive(int fd);
> -void qmp_fd_sendv(int fd, const char *fmt, va_list ap);
> -void qmp_fd_send(int fd, const char *fmt, ...);
> -QDict *qmp_fdv(int fd, const char *fmt, va_list ap);
> -QDict *qmp_fd(int fd, const char *fmt, ...);
> +
> +/**
> + * qmp_fd_send:
> + * @fd: Socket to write to.
> + * @msg: Fixed string to send.
> + *
> + * Send a message to the destination, without waiting for a reply.
> + */
> +void qmp_fd_send(int fd, const char *msg);
>
>  /**
>   * qtest_cb_for_every_machine:
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index ba09c949dc..0fa32928c8 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -441,26 +441,16 @@ QDict *qtest_qmp_receive(QTestState *s)
>      return qmp_fd_receive(s->qmp_fd);
>  }
>
> -/**
> - * Allow users to send a message without waiting for the reply,
> - * in the case that they choose to discard all replies up until
> - * a particular EVENT is received.
> +/*
> + * Internal code that converts from interpolated JSON into a message
> + * to send over the wire, without waiting for a reply.
>   */

"Internal code that ..." is awkward.  It's static, so of course it's
internal, and of course it's code :)

"Convert from X into Y" sounds odd to my (non-native!) ears.  "Convert
from X to Y" and "convert X into Y" don't.

"Without waiting for a reply" is kind of implied by "send", isn't it?

What about

  /*
   * Send a QMP message with %-interpolation like qobject_from_jsonv().
   */

Add parameter description to taste.  Uglify to conform to GLib
documentation style if you want.

> -void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
> +static void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>  {
>      QObject *qobj = NULL;
> -    int log = getenv("QTEST_LOG") != NULL;
>      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
> -     * resyncs */
> -    if (*fmt == '\377') {
> -        socket_send(fd, fmt, 1);
> -        assert(!fmt[1]);
> -        return;
> -    }
>      assert(*fmt);

Is this assertion (still) useful?  Why can't we leave the job to
qobject_from_jsonv()?

>
>      /*
> @@ -468,25 +458,17 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>       * is used.  We interpolate through QObject rather than sprintf in
>       * order to escape strings properly.
>       */
> -    if (strchr(fmt, '%')) {
> -        qobj = qobject_from_jsonv(fmt, ap);
> -        qstr = qobject_to_json(qobj);
> -    } else {
> -        qstr = qstring_from_str(fmt);
> +    if (!strchr(fmt, '%')) {
> +        qmp_fd_send(fd, fmt);
> +        return;
>      }
>
> -    /*
> -     * 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');
> +    qobj = qobject_from_jsonv(fmt, ap);
> +    qstr = qobject_to_json(qobj);
>      str = qstring_get_str(qstr);
>
> -    if (log) {
> -        fprintf(stderr, "%s", str);
> -    }
>      /* Send QMP request */
> -    socket_send(fd, str, qstring_get_length(qstr));
> +    qmp_fd_send(fd, str);
>
>      QDECREF(qstr);
>      qobject_decref(qobj);
> @@ -497,13 +479,6 @@ void qtest_async_qmpv(QTestState *s, const char *fmt, 
> va_list ap)
>      qmp_fd_sendv(s->qmp_fd, fmt, ap);
>  }
>
> -QDict *qmp_fdv(int fd, const char *fmt, va_list ap)
> -{
> -    qmp_fd_sendv(fd, fmt, ap);
> -
> -    return qmp_fd_receive(fd);
> -}
> -
>  QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>  {
>      qtest_async_qmpv(s, fmt, ap);
> @@ -512,24 +487,20 @@ QDict *qtest_qmpv(QTestState *s, const char *fmt, 
> va_list ap)
>      return qtest_qmp_receive(s);
>  }
>
> -QDict *qmp_fd(int fd, const char *fmt, ...)
> +void qmp_fd_send(int fd, const char *msg)
>  {
> -    va_list ap;
> -    QDict *response;
> +    int log = getenv("QTEST_LOG") != NULL;
>
> -    va_start(ap, fmt);
> -    response = qmp_fdv(fd, fmt, ap);
> -    va_end(ap);
> -    return response;
> -}
> -
> -void qmp_fd_send(int fd, const char *fmt, ...)
> -{
> -    va_list ap;
> -
> -    va_start(ap, fmt);
> -    qmp_fd_sendv(fd, fmt, ap);
> -    va_end(ap);
> +    if (log) {
> +        fprintf(stderr, "%s\n", msg);
> +    }
> +    /* Send QMP request */
> +    socket_send(fd, msg, strlen(msg));
> +    /*
> +     * BUG: QMP doesn't react to input until it sees a newline, an
> +     * object, or an array.  Work-around: give it a newline.
> +     */
> +    socket_send(fd, "\n", 1);

Hmm.

Before this series, qmp_fd_sendv() first parses @fmt with
%-interpolation from @ap, then converts the result back to JSON text.
Any newlines are lost, so we have to append one.

PATCH 10 adds a shortcut when @fmt doesn't contain '%'.  Newlines are
not lost in that case.  We add one anyway.  Ugh.

This patch drops the non-shortcut case.

I think qmp_fd_send() should send exactly @msg, and the newline
appending should move to qmp_fd_sendv(), where the newline loss now
happens.

>  }
>
>  QDict *qtest_qmp(QTestState *s, const char *fmt, ...)
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 839481e49b..ae0da6c9ac 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -122,9 +122,7 @@ static void GCC_FMT_ATTR(3, 0) qga_sendv(const 
> TestFixture *fixture,
>      QString *qstr = qobject_to_json(obj);
>      const char *str;
>
> -    qstring_append_chr(qstr, '\n');

Was appending a newline necessary before this patch?

It will be if you change qmp_fd_send() as I proposed.

>      str = qstring_get_str(qstr);
> -    assert(!strchr(str, '%'));
>      qmp_fd_send(fixture->fd, str);
>      QDECREF(qstr);
>      qobject_decref(obj);