Re: [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json

[Markus Armbruster <armbru@redhat.com>]

Juan Quintela <quintela@redhat.com> writes:

> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>> On Tue, Oct 10, 2017 at 08:15:42PM +0200, Juan Quintela wrote:
>>> We use int for everything (int64_t), and then we check that value is
>>> between 0 and 255.  Change it to the valid types.
>>> 
>>> For qmp, the only real change is that now max_bandwidth allows to use
>>> the k/M/G suffixes.

Are you sure?

QAPI type 'size' is integer in JSON.  No suffixes.  The QObject input
visitor does suffixes only when visiting v->keyval is true.

>> So on input apps previous would use:
>>
>>     "max-bandwidth":  1024
>>
>> ie json numeric field, and would expect to see the same when reading
>> data back from QEMU.
>>
>> With this change they could use a string field
>>
>>     "max-bandwidth":  "1k"
>
> Actually it is worse than that
>
>
> if you set:
>
>      "max-bandwidth":  1024
>
> it understand 1024M, and it outputs that big number
>
>       "max-bandwidth":  $((1024*1024*1024))
>
> (no, I don't know the value from memory)
>
> And yes, for migrate_set_parameter, we introduced it on 2.10.  We should
> have done the right thing, but  I didn't catch the error (Markus did,
> but too late, release were already done)

I suspect you're talking about *HMP*.  hmp_migrate_set_parameter(), to
be precise:

    case MIGRATION_PARAMETER_MAX_BANDWIDTH:
        p->has_max_bandwidth = true;
        /*
         * Can't use visit_type_size() here, because it
         * defaults to Bytes rather than Mebibytes.
         */
        ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
        if (ret < 0 || valuebw > INT64_MAX
            || (size_t)valuebw != valuebw) {
            error_setg(&err, "Invalid size %s", valuestr);
            break;
        }
        p->max_bandwidth = valuebw;
        break;

>> As long as QEMU's JSON parser accepts both number & string values
>> for the 'size' type it is still backwards compatible if an app
>> continues to use 1024 instead of "1k"
>>
>> On *output* though (ie 'info migrate-parameters') this is not
>> compatible for applications, unless QEMU *always* uses the
>> numeric format when generating values. ie it must always
>> report 1024, and never "1k", as apps won't be expecting a string
>> with suffix.  I can't 100% tell whether this is the case or not,
>> so CC'ing Markus to confirm if changing "int" to "size" is
>> guaranteed back-compat in both directions
>
> This is why I asked.  My *understanding* was that my changes are NOP
> if you use the old interface, but I don't claim to be an expert on QAPI.
>
> (qemu) migrate_set_parameter 100
> (qemu) info migrate_parameters 
> ...
> max-bandwidth: 104857600 bytes/second
> ...
> (qemu) migrate_set_parameter max-bandwidth 1M
> (qemu) info migrate_parameters 
> ...
> max-bandwidth: 1048576 bytes/second
> ...
> (qemu) 
>
> This is the output with my changes applied, so I think that it works
> correctly on your example.

This is HMP.  Not a stable interface.  QMP is a stable interface, but it
should not be affected by this patch.  Is your commit message
misleading?