Re: [Qemu-block] [Qemu-devel] [RFC PATCH 15/56] migration: Make XBZRLE cache size unsigned in QAPI/QMP

[Markus Armbruster]

Juan Quintela <address@hidden> writes:

> Markus Armbruster <address@hidden> wrote:
>> Sizes should use QAPI type 'size' (uint64_t).  migrate-set-cache-size
>> parameter @value is 'int' (int64_t).  qmp_migrate_set_cache_size()
>> ensures it fits into size_t.  page_cache.c implicitly converts the
>> signed size to unsigned types (it can't quite decide whether to use
>> uint64_t or size_t for cache offsets, but that's not something I can
>> tackle now).
>>
>> XBZRLECacheStats member @cache-size and query-migrate-cache-size's
>> result are also 'int'.
>>
>> Change the migrate-set-cache-size parameter and the XBZRLECacheStats
>> members to 'size', fix up hmp_migrate_set_cache_size(), and tweak a
>> few variable types to reduce implicit conversions.
>>
>> migrate-set-cache-size now accepts size values between 2^63 and
>> 2^64-1.  It accepts negative values as before, because that's how the
>> QObject input visitor works for backward compatibility.
>>
>> So does HMP's migrate_set_cache_size.
>>
>> query-migrate and query-migrate-cache-size now report cache sizes
>> above 2^63-1 correctly instead of their (negative) two's complement.
>>
>> So does HMP's "info migrate_cache_size".  HMP's "info migrate" already
>> reported the cache size correctly, because it printed the signed
>> integer with PRIu32.
>>
>
> Reviewed-by: Juan Quintela <address@hidden>
>
>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index c8cceb9..ecabff6 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -646,7 +646,7 @@
>>  # Since: 1.2
>>  ##
>>  { 'struct': 'XBZRLECacheStats',
>> -  'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
>> +  'data': {'cache-size': 'size', 'bytes': 'int', 'pages': 'int',
>>             'cache-miss': 'int', 'cache-miss-rate': 'number',
>>             'overflow': 'int' } }
>>  
>> @@ -2875,7 +2875,7 @@
>>  # <- { "return": {} }
>>  #
>>  ##
>> -{ 'command': 'migrate-set-cache-size', 'data': {'value': 'int'} }
>> +{ 'command': 'migrate-set-cache-size', 'data': {'value': 'size'} }
>>  
>>  ##
>>  # @query-migrate-cache-size:
>> @@ -2892,7 +2892,7 @@
>>  # <- { "return": 67108864 }
>>  #
>>  ##
>> -{ 'command': 'query-migrate-cache-size', 'returns': 'int' }
>> +{ 'command': 'query-migrate-cache-size', 'returns': 'size' }
>>  
>>  ##
>>  # @ObjectPropertyInfo:
>
> I am ussming this bits are backward compatible (I don't understand QMP
> to assure this)

I guess I should've explained this in the cover letter.

Until recent commit 5923f85, integers between INT64_MAX+1 and UINT64_MAX
did not work in QMP.  QEMU sent and accepted integers between INT64_MIN
and -1 instead.

The fix maintains strict compatibility for QMP input: negative integers
are accepted as before for backward compatibility.  Perhaps we can get
rid of this wart some day.

It does not maintain strict compatibility for QMP output: we now output
the correct integer.  We figure that's tolerable, because the obvious
way to parse the old output is strtoull(), and that does the right thing
for the new output when it does the right thing for the old output.

Fixing a QAPI type from 'int' to 'size' has the same compatibility
impact.

Questions?