Re: [PATCH v3 1/3] qcow2: introduce Qcow2Metadata structure

[Vladimir Sementsov-Ogievskiy]

14.01.2020 11:22, Andrey Shinkevich wrote:
> The preliminary patch to provide an extendable structure for dumping
> QCOW2 metadata allocations in image. The command line optional key is
> introduced in the patch that follows.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: Andrey Shinkevich <address@hidden>
> ---
>   qapi/block-core.json | 209 
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 208 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ff5e5e..fab7435 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -176,6 +176,209 @@
>              '*backing-image': 'ImageInfo',
>              '*format-specific': 'ImageInfoSpecific' } }
>   
> +
> +##
> +# @Qcow2Metadata:
> +#
> +# Encapsulates QCOW2 metadata information
> +#
> +# @qcow2-header: QCOW2 header info
> +#
> +# @active-l1: L1 active table info
> +#
> +# @refcount-table: refcount table info
> +#
> +# @crypt-header: encryption header info
> +#
> +# @bitmaps: bitmap tables info
> +#
> +# @snapshot-table: snapshot tables info
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Metadata',
> +  'data': { 'qcow2-header': 'Qcow2Header',
> +            'refcount-table': 'Qcow2RefcountTable',
> +            'active-l1': 'Qcow2L1Table',
> +            '*crypt-header': 'Qcow2EncryptionHeader',
> +            '*bitmaps': 'Qcow2Bitmaps',
> +            '*snapshot-table': 'Qcow2SnapshotsTable' } }
> +
> +##
> +# @Qcow2Header:
> +#
> +# QCOW2 header information
> +#
> +# @version: version number
> +#
> +# @location: header offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Header',
> +  'data': {'version': 'int',
> +           'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2EncryptionHeader:
> +#
> +# QCOW2 encryption header information
> +#
> +# @location: header offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2EncryptionHeader',
> +  'data': {'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2RefcountTable:
> +#
> +# QCOW2 refcount table information
> +#
> +# @location: refcount table offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2RefcountTable',
> +  'data': {'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2L1Table:
> +#
> +# L1 table information
> +#
> +# @l2-list: list of L2 table locations
> +#
> +# @location: L1 table offset and size in image
> +#
> +# @name: entity name the table relates to
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2L1Table',
> +  'data': {'l2-list': ['Qcow2Allocation'],

If we list only allocated l2, we lost information about their
idexes.

I think we need instead

'entries': ['Qcow2L1TableEntry'],


> +           'location': 'Qcow2Allocation',
> +           'name': 'str'} }

L1 table doesn't has name. Name is a property of snapshot.
I'd prefer to follow object model defined in docs/interop/qcow2.txt as
close as possible, with the only exception for "location" property, which
may be addeded to data structures like Qcow2L1Table.. (and I don't think
we need to add location into each Qcow2L1TableEntry), and dropping
corresponding offset and size fields, which are mirrored to the location
object.

Generic location object is good to automatically parse resulting json, to
make a view of it.

> +
> +##
> +# @Qcow2Allocation:
> +#
> +# QCOW2 data location in image
> +#
> +# @offset: data offset
> +#
> +# @size: data size
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Allocation',
> +  'data': {'offset': 'uint64', 'size': 'uint64' } }
> +
> +##
> +# @Qcow2Bitmaps:

This may be called Qcow2BitmapsExtension, to follow qcow2.txt

> +#
> +# QCOW2 bitmaps information
> +#
> +# @nb-bitmaps: the number of bitmaps contained in the image
> +#
> +# @bitmap-dir: bitmap directory information
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Bitmaps',
> +  'data': {'nb-bitmaps': 'int',
> +           'bitmap-dir': 'Qcow2BitmapDir' } }

Hmm I don't like these abbreviations. Qcow2BitmapDirectory will
interfere with existing type, I understand but let's at least keep
full field names, like bitmap-directory.

Also, wouldn't existence of types Qcow2BitmapDir and Qcow2BitmapDirectory
be confusing? I think it will.

Maybe, for qapi, we need QapiQcow2BitmapDirectory, to make it obvious?

[upd, after looking ahead], it should be called Qcow2BitmapDirectoryInfo.

> +
> +##
> +# @Qcow2BitmapDir:
> +#
> +# QCOW2 bitmap directory information
> +#
> +# @dir-entries: list of bitmap directory entries
> +#
> +# @location: bitmap directory offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapDir',
> +  'data': {'dir-entries': ['Qcow2BitmapDirectoryEntry'],
> +           'location': 'Qcow2Allocation' } }

I'd call them just 'entries'. It inconsistent with locations,
and I didn't see somewhere in qapi prefixing fields by structure
name abbreviation.

> +
> +##
> +# @Qcow2BitmapDirectoryEntry:
> +#
> +# QCOW2 bitmap directory entry information
> +#
> +# @bitmap-table: bitmap table offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapDirectoryEntry',
> +  'data': {'bitmap-table': 'Qcow2BitmapTableInfo',
> +           'bitmap-name': 'str' } }

and here, I'd keep "bitmap-table", as it matches qcow2.txt,
but s/bitmap-name/name.

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

Keep Info the last word, Qcow2BitmapTableEntryInfo

> +#
> +# QCOW2 bitmap table entry information
> +#
> +# @type: bitmap table entry type
> +#
> +# @cluster: bitmap table entry offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapTableInfoEntry',
> +  'data': {'type': 'Qcow2BitmapTableInfoEntryType',
> +           '*cluster': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2BitmapTableInfoEntryType:
> +#
> +# An enumeration of cluster types in bitmap table
> +#
> +# @all-zeroes: cluster should be read as all zeroes
> +#
> +# @all-ones: cluster should be read as all ones
> +#
> +# @serialized: cluster data are written on disk
> +#
> +# Since: 5.0
> +##
> +{ 'enum': 'Qcow2BitmapTableInfoEntryType',
> +  'data': ['all-zeroes', 'all-ones', 'serialized'] }
> +
> +##
> +# @Qcow2SnapshotsTable:
> +#
> +# Snapshots table location in image file.
> +#
> +# @location: offset and size of snapshot table
> +#
> +# @l1-list: list of snapshots L1 tables
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2SnapshotsTable',

in qcow2.txt it called snapshot table.. So, I don't think we need 's'.

> +  'data': {'location': 'Qcow2Allocation',
> +           'l1-list': ['Qcow2L1Table'] } }


instead, we need 'entries': ['Qcow2SnapshotTableEntry']

And the entry will contain at least l1-table and name.

> +
>   ##
>   # @ImageCheck:
>   #
> @@ -215,6 +418,9 @@
>   #                       field is present if the driver for the image format
>   #                       supports it
>   #
> +# @metadata: encapsulates QCOW2 tables allocation information (default: none,
> +#            turned on with the command line optional key; since 5.0)
> +#
>   # Since: 1.4
>   #
>   ##
> @@ -223,7 +429,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',
> +           '*metadata': 'Qcow2Metadata' } }
>   
>   ##
>   # @MapEntry:
> 


-- 
Best regards,
Vladimir