[PATCH 0/2] block: Remove unused BlockDeviceMapEntry

[PATCH 0/2] block: Remove unused BlockDeviceMapEntry

[Max Reitz]

Hi,

Markus has revived a rather old patch to remove an unused QAPI
structure:

https://lists.nongnu.org/archive/html/qemu-block/2020-10/msg01902.html

He quoted a response of mine to the original patch, where I noted that
removing this structure is OK because it is superseded by another
structure (MapEntry), but that other structure’s documentation is worse.
So we should merge some of BlockDeviceMapEntry’s documentation into
MapEntry before removing it.

This series combines said merge with Markus’s patch to drop
BlockDeviceMapEntry.


Markus Armbruster (1):
  block: Remove unused BlockDeviceMapEntry

Max Reitz (1):
  qapi/block-core: Improve MapEntry documentation

 qapi/block-core.json | 47 ++++++++++++--------------------------------
 1 file changed, 13 insertions(+), 34 deletions(-)

-- 
2.28.0

[PATCH 1/2] qapi/block-core: Improve MapEntry documentation

[Max Reitz]

MapEntry and BlockDeviceMapEntry are kind of the same thing, and the
latter is not used, so we want to remove it.  However, the documentation
it provides for some fields is better than that of MapEntry, so steal
some of it for the latter.

(And adjust them a bit in the process, because I feel like we can make
them even clearer.)

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1b8b4156b4..3f86675357 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -244,17 +244,25 @@
 #
 # Mapping information from a virtual block range to a host file range
 #
-# @start: the start byte of the mapped virtual range
+# @start: virtual (guest) offset of the first byte described by this
+#         entry
 #
 # @length: the number of bytes of the mapped virtual range
 #
-# @data: whether the mapped range has data
+# @data: reading the image will actually read data from a file (in
+#        particular, if @offset is present this means that the sectors
+#        are not simply preallocated, but contain actual data in raw
+#        format)
 #
-# @zero: whether the virtual blocks are zeroed
+# @zero: whether the virtual blocks read as zeroes
 #
-# @depth: the depth of the mapping
+# @depth: number of layers (0 = top image, 1 = top image's backing
+#         file, ..., n - 1 = bottom image (where n is the number of
+#         images in the chain)) before reaching one for which the
+#         range is allocated
 #
-# @offset: the offset in file that the virtual sectors are mapped to
+# @offset: if present, the image file stores the data for this range
+#          in raw format at the given (host) offset
 #
 # @filename: filename that is referred to by @offset
 #
-- 
2.28.0

Re: [PATCH 1/2] qapi/block-core: Improve MapEntry documentation

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> MapEntry and BlockDeviceMapEntry are kind of the same thing, and the
> latter is not used, so we want to remove it.  However, the documentation
> it provides for some fields is better than that of MapEntry, so steal
> some of it for the latter.
>
> (And adjust them a bit in the process, because I feel like we can make
> them even clearer.)
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-core.json | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1b8b4156b4..3f86675357 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -244,17 +244,25 @@
>  #
>  # Mapping information from a virtual block range to a host file range
>  #
> -# @start: the start byte of the mapped virtual range
> +# @start: virtual (guest) offset of the first byte described by this
> +#         entry
>  #
>  # @length: the number of bytes of the mapped virtual range
>  #
> -# @data: whether the mapped range has data
> +# @data: reading the image will actually read data from a file (in
> +#        particular, if @offset is present this means that the sectors
> +#        are not simply preallocated, but contain actual data in raw
> +#        format)
>  #
> -# @zero: whether the virtual blocks are zeroed
> +# @zero: whether the virtual blocks read as zeroes
>  #
> -# @depth: the depth of the mapping
> +# @depth: number of layers (0 = top image, 1 = top image's backing
> +#         file, ..., n - 1 = bottom image (where n is the number of
> +#         images in the chain)) before reaching one for which the
> +#         range is allocated
>  #
> -# @offset: the offset in file that the virtual sectors are mapped to
> +# @offset: if present, the image file stores the data for this range
> +#          in raw format at the given (host) offset
>  #
>  # @filename: filename that is referred to by @offset
>  #

I can't vouch for the comment's accuracy without reading a lot of code,
but I can say that the comment is clearly much clearer, so:

Acked-by: Markus Armbruster <armbru@redhat.com>

[PATCH 2/2] block: Remove unused BlockDeviceMapEntry

[Max Reitz]

From: Markus Armbruster <armbru@redhat.com>

BlockDeviceMapEntry has never been used.  It was added in commit
facd6e2 "so that it is published through the introspection mechanism."
What exactly introspecting types that aren't used for anything could
accomplish isn't clear.  What "introspection mechanism" to use is also
nebulous.  To the best of my knowledge, there has never been one that
covered this type.  Certainly not query-qmp-schema, which includes
only types that are actually used in QMP.

Not being able to introspect BlockDeviceMapEntry hasn't bothered
anyone enough to complain in almost four years.  Get rid of it.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/block-core.json | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3f86675357..04ad80bc1e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -426,35 +426,6 @@
 ##
 { 'enum': 'BlockDeviceIoStatus', 'data': [ 'ok', 'failed', 'nospace' ] }
 
-##
-# @BlockDeviceMapEntry:
-#
-# Entry in the metadata map of the device (returned by "qemu-img map")
-#
-# @start: Offset in the image of the first byte described by this entry
-#         (in bytes)
-#
-# @length: Length of the range described by this entry (in bytes)
-#
-# @depth: Number of layers (0 = top image, 1 = top image's backing file, etc.)
-#         before reaching one for which the range is allocated.  The value is
-#         in the range 0 to the depth of the image chain - 1.
-#
-# @zero: the sectors in this range read as zeros
-#
-# @data: reading the image will actually read data from a file (in particular,
-#        if @offset is present this means that the sectors are not simply
-#        preallocated, but contain actual data in raw format)
-#
-# @offset: if present, the image file stores the data for this range in
-#          raw format at the given offset.
-#
-# Since: 1.7
-##
-{ 'struct': 'BlockDeviceMapEntry',
-  'data': { 'start': 'int', 'length': 'int', 'depth': 'int', 'zero': 'bool',
-            'data': 'bool', '*offset': 'int' } }
-
 ##
 # @DirtyBitmapStatus:
 #
-- 
2.28.0

Re: [PATCH 0/2] block: Remove unused BlockDeviceMapEntry

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Hi,
>
> Markus has revived a rather old patch to remove an unused QAPI
> structure:
>
> https://lists.nongnu.org/archive/html/qemu-block/2020-10/msg01902.html
>
> He quoted a response of mine to the original patch, where I noted that
> removing this structure is OK because it is superseded by another
> structure (MapEntry), but that other structure’s documentation is worse.
> So we should merge some of BlockDeviceMapEntry’s documentation into
> MapEntry before removing it.
>
> This series combines said merge with Markus’s patch to drop
> BlockDeviceMapEntry.

Queued, thanks!