qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 06/56] test-qga: Clean up how we test QGA synchr


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 06/56] test-qga: Clean up how we test QGA synchronization
Date: Fri, 10 Aug 2018 15:57:44 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)

Eric Blake <address@hidden> writes:

> On 08/08/2018 07:02 AM, Markus Armbruster wrote:
>> To permit recovering from arbitrary JSON parse errors, the JSON parser
>> resets itself on lexical errors.  We recommend sending a 0xff byte for
>> that purpose, and test-qga covers this usage since commit 5229564b832.
>> That commit had to add an ugly hack to qmp_fd_vsend() to make capable
>> of sending this byte (it's designed to send only valid JSON).
>>
>> The previous commit added a way to send arbitrary text.  Put that to
>> use for this purpose, and drop the hack from qmp_fd_vsend().
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   tests/libqtest.c | 39 +++++++++++++++++++++------------------
>>   tests/libqtest.h |  2 ++
>>   tests/test-qga.c |  8 ++++----
>>   3 files changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index c02fc91b37..9c844874e4 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -489,16 +489,6 @@ void qmp_fd_vsend(int fd, const char *fmt, va_list ap)
>>   {
>>       QObject *qobj;
>>   -    /*
>> -     * qobject_from_vjsonf_nofail() chokes on leading 0xff as invalid
>> -     * JSON, but tests/test-qga.c needs to send that to test QGA
>> -     * synchronization
>> -     */
>> -    if (*fmt == '\377') {
>> -        socket_send(fd, fmt, 1);
>> -        fmt++;
>> -    }
>> -
>>       /* Going through qobject ensures we escape strings properly */
>>       qobj = qobject_from_vjsonf_nofail(fmt, ap);
>
> This does JSON interpolation...
>
>>   @@ -586,23 +576,36 @@ void qtest_qmp_send(QTestState *s, const
>> char *fmt, ...)
>>       va_end(ap);
>>   }
>>   -void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...)
>> +void qmp_fd_vsend_raw(int fd, const char *fmt, va_list ap)
>>   {
>>       bool log = getenv("QTEST_LOG") != NULL;
>> -    va_list ap;
>> -    char *str;
>> -
>> -    va_start(ap, fmt);
>> -    str = g_strdup_vprintf(fmt, ap);
>> -    va_end(ap);
>> +    char *str = g_strdup_vprintf(fmt, ap);
>
> ...while the new code does printf interpolation...
>
>> +++ b/tests/test-qga.c
>> @@ -147,10 +147,10 @@ static void test_qga_sync_delimited(gconstpointer fix)
>>       unsigned char c;
>>       QDict *ret;
>>   -    qmp_fd_send(fixture->fd,
>> -                "\xff{'execute': 'guest-sync-delimited',"
>> -                " 'arguments': {'id': %u } }",
>> -                r);
>> +    qmp_fd_send_raw(fixture->fd,
>> +                    "\xff{'execute': 'guest-sync-delimited',"
>> +                    " 'arguments': {'id': %u } }",
>> +                    r);
>
> ...and your test was relying on interpolation. Fortunately, your only

Yes, my patch is a bit lazy there.

> thing being interpolated (%u) happens to result in valid JSON - but
> you may want to split this into:
>
> qmp_fd_send_raw(fixture->fd, "\xff");
> qmp_fd_send(fixutre->fd, "{'execute ... %u}}", f);
>
> to make it less questionable. If not, then call it out in the commit
> message and/or a comment on the code itself.

Splitting is easier than explaining, so that's what I'll do.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]