Re: [Qemu-devel] [PATCH v2 15/23] tests: New helper qtest_qmp_receive_success()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/27/2018 10:13 AM, Markus Armbruster wrote:
>> Commit b21373d0713 copied wait_command() from tests/migration-test.c
>> to tests/tpm-util.c.  Replace both copies by new libqtest helper
>> qtest_qmp_receive_success().  Also use it to simplify
>> qtest_qmp_device_del().
>>
>> Bonus: gets rid of a non-literal format string.  A step towards
>> compile-time format string checking without triggering
>> -Wformat-nonliteral.
>
> Where? [1]
>
>>
>> Cc: Thomas Huth <address@hidden>
>> Cc: Juan Quintela <address@hidden>
>> Cc: Dr. David Alan Gilbert <address@hidden>
>> Cc: Stefan Berger <address@hidden>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Juan Quintela <address@hidden>
>> Reviewed-by: Stefan Berger <address@hidden>
>> ---
>
>> +++ b/tests/libqtest.c
>> @@ -1029,6 +1029,35 @@ void qtest_cb_for_every_machine(void (*cb)(const char 
>> *machine))
>>       qobject_unref(response);
>>   }
>>   +QDict *qtest_qmp_receive_success(QTestState *s,
>> +                                 void (*event_cb)(void *opaque,
>> +                                                  const char *event,
>> +                                                  QDict *data),
>> +                                 void *opaque)
>> +{
>
> I like the new signature!
>
>> +++ b/tests/migration-test.c
>
>>   /*
>>    * Events can get in the way of responses we are actually waiting for.
>>    */
>>   static QDict *wait_command(QTestState *who, const char *command)
>>   {
>
> [1] This is still taking a format non-literal command...
>
>> -    const char *event_string;
>> -    QDict *response, *ret;
>> -
>> -    response = qtest_qmp(who, command);
> ...
>> +    qtest_qmp_send(who, command);
>
> and is passing it on through.

No worse than before.  Will be fixed in the next two patches.

>> +    return qtest_qmp_receive_success(who, stop_cb, NULL);
>>   }
>>   
>
>> +++ b/tests/tpm-util.c
>
>> -/*
>> - * Events can get in the way of responses we are actually waiting for.
>> - */
>> -static QDict *tpm_util_wait_command(QTestState *who, const char *command)
>> -{
>
> Maybe you were counting this instance,
>

    -    const char *event_string;
    -    QDict *response;
    -
    -    response = qtest_qmp(who, command);

/work/armbru/qemu/tests/tpm-util.c: In function ‘tpm_util_wait_command’:
/work/armbru/qemu/tests/tpm-util.c:258:5: warning: format not a string literal 
and no format arguments [-Wformat-security]
     response = qtest_qmp(who, command);
     ^~~~~~~~

    -
    -    while (qdict_haskey(response, "event")) {
    -        /* OK, it was an event */
    -        event_string = qdict_get_str(response, "event");
    -        if (!strcmp(event_string, "STOP")) {
    -            got_stop = true;
    -        }
    -        qobject_unref(response);
    -        response = qtest_qmp_receive(who);
    -    }
    -    return response;
    -}
>>   void tpm_util_wait_for_migration_complete(QTestState *who)
>>   {
>>       while (true) {
>> -        QDict *rsp, *rsp_return;
>> +        QDict *rsp_return;
>>           bool completed;
>>           const char *status;
>>   -        rsp = tpm_util_wait_command(who, "{ 'execute':
>> 'query-migrate' }");
>> -        rsp_return = qdict_get_qdict(rsp, "return");
>> +        qtest_qmp_send(who, "{ 'execute': 'query-migrate' }");
>
> where you did indeed drop a format non-literal?

> But on the assumption that there's more cleanups later in the series,
> this is indeed incrementally better, so
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!