Re: [PATCH 1/2] qcow2: introduce Qcow2Metadata structure

[Andrey Shinkevich]

On 08/01/2020 01:07, Eric Blake wrote:
> On 12/27/19 5:43 AM, Andrey Shinkevich wrote:
>> The preliminary patch to provide an extendable structure for dumping
>> QCOW2 metadata allocations in image.
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> Signed-off-by: Andrey Shinkevich <address@hidden>
>> ---
>>   qapi/block-core.json | 208 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 207 insertions(+), 1 deletion(-)
>>
> 
>> +
>> +##
>> +# @Qcow2BitmapTbl:
>> +#
> 
> Any reason we must abbreviate instead of spelling this out as Table?
> 

Thank you Eric for your comments. The reason for the abbreviation is 
that the name 'Qcow2BitmapTable' is in conflict with the existing 
structure name in block/qcow2-bitmap.c. I can rename it to 
'Qcow2BitmapTableInfo'


>> +# QCOW2 bitmap table information
>> +#
>> +# @table-entries: list of bitmap table entries
>> +#
>> +# @location: bitmap table offset and size in image
>> +#
>> +# Since: 5.0
>> +##
>> +{ 'struct': 'Qcow2BitmapTbl',
>> +  'data': {'table-entries': ['Qcow2BitmapTblEntry'],
>> +           'location': 'Qcow2Allocation' } }
>> +
>> +##
>> +# @Qcow2BitmapTblEntry:
> 
> Similar question
> 

and to 'Qcow2BitmapTableInfoEntry' here.

> 
>> +{ 'struct': 'Qcow2BitmapTblEntry',
>> +  'data': {'type': 'Qcow2BitmapTblEntryType',
>> +           '*cluster': 'Qcow2Allocation' } }
>> +
>> +##
>> +# @Qcow2BitmapTblEntryType:
>> +#
>> +# An enumeration of cluster types in bitmap table
>> +#
>> +# @all-zeros: cluster should be read as all zeros
> 
> While there are multiple 'zeros' in the code base, 'zeroes' appears to 
> be the more common spelling.
> 
> 
>> @@ -215,6 +418,8 @@
>>   #                       field is present if the driver for the image 
>> format
>>   #                       supports it
>>   #
>> +# @viscera: encapsulates QCOW2 tables allocation information
> 
> Missing a '(since 5.0)' tag.  Interesting choice of name; not one I 
> would have picked out of the air.  Would 'metadata' be any more of a 
> reasonable name?
> 
>> +#
>>   # Since: 1.4
>>   #
>>   ##
>> @@ -223,7 +428,8 @@
>>              '*image-end-offset': 'int', '*corruptions': 'int', 
>> '*leaks': 'int',
>>              '*corruptions-fixed': 'int', '*leaks-fixed': 'int',
>>              '*total-clusters': 'int', '*allocated-clusters': 'int',
>> -           '*fragmented-clusters': 'int', '*compressed-clusters': 
>> 'int' } }
>> +           '*fragmented-clusters': 'int', '*compressed-clusters': 'int',
>> +           '*viscera': 'Qcow2Metadata' } }
> 
> The field is listed as optional, but the docs don't describe what 
> controls whether it is present or absent.  Is that worth adding?
> 
> 

-- 
With the best regards,
Andrey Shinkevich