Re: [Qemu-devel] [PATCH v2 19/23] migration-test: Clean up string interpolation into QMP, part 3

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/27/2018 10:13 AM, Markus Armbruster wrote:
>> Leaving interpolation into JSON to qmp() is more robust than building
>> QMP input manually, as explained in the recent commit "tests: Clean up
>> string interpolation into QMP input (simple cases)".
>>
>> migration-test.c interpolates strings into JSON in a few places:
>>
>> * migrate_set_parameter() interpolates string parameter @value as a
>>    JSON number.  Change it to long long.  This requires changing
>>    migrate_check_parameter() similarly.
>>
>> * migrate_set_capability() interpolates string parameter @value as a
>>    JSON boolean.  Change it to bool.
>>
>> * deprecated_set_speed() interpolates string parameter @value as a
>>    JSON number.  Change it to long long.
>>
>> Bonus: gets rid of non-literal format strings.  A step towards
>> compile-time format string checking without triggering
>> -Wformat-nonliteral.
>>
>> Cc: Juan Quintela <address@hidden>
>> Cc: Dr. David Alan Gilbert <address@hidden>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> Reviewed-by: Juan Quintela <address@hidden>
>> ---
>>   tests/migration-test.c | 74 +++++++++++++++++-------------------------
>>   1 file changed, 29 insertions(+), 45 deletions(-)
>>
>> diff --git a/tests/migration-test.c b/tests/migration-test.c
>> index 1c1e56b15b..132c30891d 100644
>> --- a/tests/migration-test.c
>> +++ b/tests/migration-test.c
>> @@ -315,31 +315,25 @@ static void cleanup(const char *filename)
>>   }
>>     static void migrate_check_parameter(QTestState *who, const char
>> *parameter,
>> -                                    const char *value)
>> +                                    long long value)
>>   {
>>       QDict *rsp_return;
>> -    char *result;
>>         rsp_return = wait_command(who,
>>                                 "{ 'execute': 'query-migrate-parameters' }");
>> -    result = g_strdup_printf("%" PRId64,
>> -                             qdict_get_try_int(rsp_return,  parameter, -1));
>> -    g_assert_cmpstr(result, ==, value);
>
> The old code allows defaulting to -1 if the value is not present;
>
>> -    g_free(result);
>> +    g_assert_cmpint(qdict_get_int(rsp_return,  parameter), ==, value);
>
> the new code requires the value to be found.  Matters if any of the
> callers passed "-1" in the old code, but a search of the file doesn't
> spot any such callers. So I think you're okay.

Also:

    ##
    # @MigrationParameters:
    #
    # The optional members aren't actually optional.
    #

This is due to commit 1bda8b3c695.

> Also, while touching this line, it would be nice to get rid of the
> double space before parameter.

Of course.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!