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

[Juan Quintela]

Markus Armbruster <address@hidden> wrote:
> Juan Quintela <address@hidden> writes:
>
>> "Daniel P. Berrange" <address@hidden> 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?

No.  That is why I ask O:-)

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

Aha.  So patch can go in.

>> 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;

Ok. Understood.  Thanks.


>>> 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?

Yeap.  The part about RFC was because I didn't really understood what
was happening here, and wanted "adult supervision".

My reading from your answer is that patch can go in, right?

Thanks, Juan.