[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 10/10] migration: [RFC] Use proper types in json |
Date: |
Wed, 08 Nov 2017 11:25:52 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Juan Quintela <address@hidden> writes:
> 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:-)
Fair enough :)
>> 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?
I think so.