On 08/09/2017 08:18 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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