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

[Eric Blake]

On 08/09/2017 08:18 AM, Markus Armbruster wrote:
> 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 later rename qmp_fd_sendv() into qtest_qmp_sendv(), in 15/22, along
with another update to its signature.  Alluding to that here won't hurt.

> 
> 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.

This would be the series to do it, though, if we have a preferred
alternative name.

> 
> 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().

At the end of the series, qmp_fd_send() has no varargs buddy.
qtest_qmp_sendv() and qtest_qmp_sendf() would be the buddies.

>> -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()?

It is still useful if...

> 
>>
>>      /*
>> @@ -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;

...we take this shortcut. But later in the series, the shortcut no
longer fires (and at that time, I delete the assert).


>> +    /* 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.

In fact, I got rid of the \n hack here in 13/22.  But for one patch
longer, I could keep the workaround in qmp_fd_sendv(), rather than the
churn of moving it to qmp_fd_send() just to delete it in the next patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature