[PATCH 0/4] block/file: Show extent size in qemu-img info

[PATCH 0/4] block/file: Show extent size in qemu-img info

[Hanna Reitz]

Hi,

This series makes qemu-img info print the extent size of an image file
(if available).  To do so, we have to do a couple of things:
1. Add a .bdrv_get_specific_info handler to the file-posix driver
   (patch 4)
2. Have bdrv_query_image_info() collect this driver-specific info not
   only on the format level, but also on the protocol level (if there is
   an unambiguous node) so we actually get the information from the
   file-posix node when querying whatever format node is on top
   (patch 2)
3. Have bdrv_image_info_dump() print this protocol-level information
   (patch 3)

Extent size informations seems unavailable on ext4 at least, so if we
did just this, you would see the following for an image on ext4:

```
$ qemu-img info -f raw test.img
image: test.img
file format: raw
virtual size: 1 MiB (1048576 bytes)
disk size: 4 KiB
Protocol specific information:
```

That last part looks a bit strange -- it's a heading without a section.
So patch 1 makes bdrv_image_info_specific_dump() omit that heading if
there is no information that can be printed.


Hanna Reitz (4):
  block: Improve empty format-specific info dump
  block: Add protocol-specific image info
  block: Print protocol-specific information
  block/file: Add file-specific image info

 qapi/block-core.json | 32 ++++++++++++++++++++--
 include/block/qapi.h |  3 +-
 block/file-posix.c   | 30 ++++++++++++++++++++
 block/qapi.c         | 65 +++++++++++++++++++++++++++++++++++++++++---
 qemu-io-cmds.c       |  4 +--
 5 files changed, 124 insertions(+), 10 deletions(-)

-- 
2.35.1

[PATCH 1/4] block: Improve empty format-specific info dump

[Hanna Reitz]

When a block driver supports obtaining format-specific information, but
that object only contains optional fields, it is possible that none of
them are present, so that dump_qobject() (called by
bdrv_image_info_specific_dump()) will not print anything.

The callers of bdrv_image_info_specific_dump() put a header above this
information ("Format specific information:\n"), which will look strange
when there is nothing below.  Modify bdrv_image_info_specific_dump() to
print this header instead of its callers, and only if there is indeed
something to be printed.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 include/block/qapi.h |  3 ++-
 block/qapi.c         | 41 +++++++++++++++++++++++++++++++++++++----
 qemu-io-cmds.c       |  4 ++--
 3 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/include/block/qapi.h b/include/block/qapi.h
index 22c7807c89..c09859ea78 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -40,6 +40,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
                            Error **errp);
 
 void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
-void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec);
+void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
+                                   const char *prefix);
 void bdrv_image_info_dump(ImageInfo *info);
 #endif
diff --git a/block/qapi.c b/block/qapi.c
index cf557e3aea..51202b470a 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -777,7 +777,35 @@ static void dump_qdict(int indentation, QDict *dict)
     }
 }
 
-void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec)
+/*
+ * Return whether dumping the given QObject with dump_qobject() would
+ * yield an empty dump, i.e. not print anything.
+ */
+static bool qobject_is_empty_dump(const QObject *obj)
+{
+    switch (qobject_type(obj)) {
+    case QTYPE_QNUM:
+    case QTYPE_QSTRING:
+    case QTYPE_QBOOL:
+        return false;
+
+    case QTYPE_QDICT:
+        return qdict_size(qobject_to(QDict, obj)) == 0;
+
+    case QTYPE_QLIST:
+        return qlist_empty(qobject_to(QList, obj));
+
+    default:
+        abort();
+    }
+}
+
+/**
+ * Dumps the given ImageInfoSpecific object in a human-readable form,
+ * prepending an optional prefix if the dump is not empty.
+ */
+void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
+                                   const char *prefix)
 {
     QObject *obj, *data;
     Visitor *v = qobject_output_visitor_new(&obj);
@@ -785,7 +813,12 @@ void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec)
     visit_type_ImageInfoSpecific(v, NULL, &info_spec, &error_abort);
     visit_complete(v, &obj);
     data = qdict_get(qobject_to(QDict, obj), "data");
-    dump_qobject(1, data);
+    if (!qobject_is_empty_dump(data)) {
+        if (prefix) {
+            qemu_printf("%s", prefix);
+        }
+        dump_qobject(1, data);
+    }
     qobject_unref(obj);
     visit_free(v);
 }
@@ -866,7 +899,7 @@ void bdrv_image_info_dump(ImageInfo *info)
     }
 
     if (info->has_format_specific) {
-        qemu_printf("Format specific information:\n");
-        bdrv_image_info_specific_dump(info->format_specific);
+        bdrv_image_info_specific_dump(info->format_specific,
+                                      "Format specific information:\n");
     }
 }
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2f0d8ac25a..084ec44d3b 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1819,8 +1819,8 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
         return -EIO;
     }
     if (spec_info) {
-        printf("Format specific information:\n");
-        bdrv_image_info_specific_dump(spec_info);
+        bdrv_image_info_specific_dump(spec_info,
+                                      "Format specific information:\n");
         qapi_free_ImageInfoSpecific(spec_info);
     }
 
-- 
2.35.1

[PATCH 2/4] block: Add protocol-specific image info

[Hanna Reitz]

The ImageInfo object currently only contains (optional) format-specific
image information.  However, perhaps the protocol node can provide some
additional information, so add a new field presenting it.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 qapi/block-core.json |  6 +++++-
 block/qapi.c         | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index beeb91952a..e7d6c2e0cc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -236,6 +236,9 @@
 # @format-specific: structure supplying additional format-specific
 #                   information (since 1.7)
 #
+# @protocol-specific: structure supplying additional protocol-specific
+#                     information (since 7.1)
+#
 # Since: 1.3
 #
 ##
@@ -246,7 +249,8 @@
            '*backing-filename': 'str', '*full-backing-filename': 'str',
            '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
            '*backing-image': 'ImageInfo',
-           '*format-specific': 'ImageInfoSpecific' } }
+           '*format-specific': 'ImageInfoSpecific',
+           '*protocol-specific': 'ImageInfoSpecific' } }
 
 ##
 # @ImageCheck:
diff --git a/block/qapi.c b/block/qapi.c
index 51202b470a..293983cf82 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -262,6 +262,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
     int64_t size;
     const char *backing_filename;
     BlockDriverInfo bdi;
+    BlockDriverState *protocol_bs;
     int ret;
     Error *err = NULL;
     ImageInfo *info;
@@ -303,6 +304,24 @@ void bdrv_query_image_info(BlockDriverState *bs,
     }
     info->has_format_specific = info->format_specific != NULL;
 
+    /* Try to look for an unambiguous protocol node */
+    protocol_bs = bs;
+    while (protocol_bs && !QLIST_EMPTY(&protocol_bs->children)) {
+        protocol_bs = bdrv_primary_bs(protocol_bs);
+    }
+    if (protocol_bs) {
+        /* Assert that this is a protocol node */
+        assert(QLIST_EMPTY(&protocol_bs->children));
+
+        info->protocol_specific = bdrv_get_specific_info(protocol_bs, &err);
+        if (err) {
+            error_propagate(errp, err);
+            qapi_free_ImageInfo(info);
+            goto out;
+        }
+        info->has_protocol_specific = info->protocol_specific != NULL;
+    }
+
     backing_filename = bs->backing_file;
     if (backing_filename[0] != '\0') {
         char *backing_filename2;
-- 
2.35.1

[PATCH 3/4] block: Print protocol-specific information

[Hanna Reitz]

Make bdrv_image_info_dump() print protocol-specific information.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/qapi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/qapi.c b/block/qapi.c
index 293983cf82..169ea08f70 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -921,4 +921,9 @@ void bdrv_image_info_dump(ImageInfo *info)
         bdrv_image_info_specific_dump(info->format_specific,
                                       "Format specific information:\n");
     }
+
+    if (info->has_protocol_specific) {
+        bdrv_image_info_specific_dump(info->protocol_specific,
+                                      "Protocol specific information:\n");
+    }
 }
-- 
2.35.1

[PATCH 4/4] block/file: Add file-specific image info

[Hanna Reitz]

Add some (optional) information that the file driver can provide for
image files, namely the extent size.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 qapi/block-core.json | 26 ++++++++++++++++++++++++--
 block/file-posix.c   | 30 ++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e7d6c2e0cc..728da051ae 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -139,16 +139,29 @@
       '*encryption-format': 'RbdImageEncryptionFormat'
   } }
 
+##
+# @ImageInfoSpecificFile:
+#
+# @extent-size: Extent size (if available)
+#
+# Since: 7.1
+##
+{ 'struct': 'ImageInfoSpecificFile',
+  'data': {
+      '*extent-size': 'size'
+  } }
+
 ##
 # @ImageInfoSpecificKind:
 #
 # @luks: Since 2.7
 # @rbd: Since 6.1
+# @file: Since 7.1
 #
 # Since: 1.7
 ##
 { 'enum': 'ImageInfoSpecificKind',
-  'data': [ 'qcow2', 'vmdk', 'luks', 'rbd' ] }
+  'data': [ 'qcow2', 'vmdk', 'luks', 'rbd', 'file' ] }
 
 ##
 # @ImageInfoSpecificQCow2Wrapper:
@@ -185,6 +198,14 @@
 { 'struct': 'ImageInfoSpecificRbdWrapper',
   'data': { 'data': 'ImageInfoSpecificRbd' } }
 
+##
+# @ImageInfoSpecificFileWrapper:
+#
+# Since: 7.1
+##
+{ 'struct': 'ImageInfoSpecificFileWrapper',
+  'data': { 'data': 'ImageInfoSpecificFile' } }
+
 ##
 # @ImageInfoSpecific:
 #
@@ -199,7 +220,8 @@
       'qcow2': 'ImageInfoSpecificQCow2Wrapper',
       'vmdk': 'ImageInfoSpecificVmdkWrapper',
       'luks': 'ImageInfoSpecificLUKSWrapper',
-      'rbd': 'ImageInfoSpecificRbdWrapper'
+      'rbd': 'ImageInfoSpecificRbdWrapper',
+      'file': 'ImageInfoSpecificFileWrapper'
   } }
 
 ##
diff --git a/block/file-posix.c b/block/file-posix.c
index bfd9b21111..4323345c99 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3068,6 +3068,34 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
+static ImageInfoSpecific *raw_get_specific_info(BlockDriverState *bs,
+                                                Error **errp)
+{
+    BDRVRawState *s = bs->opaque;
+    ImageInfoSpecificFile *file_info = g_new0(ImageInfoSpecificFile, 1);
+    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
+
+    *spec_info = (ImageInfoSpecific){
+        .type = IMAGE_INFO_SPECIFIC_KIND_FILE,
+        .u.file.data = file_info,
+    };
+
+#ifdef FS_IOC_FSGETXATTR
+    {
+        struct fsxattr attr;
+        int ret;
+
+        ret = ioctl(s->fd, FS_IOC_FSGETXATTR, &attr);
+        if (!ret && attr.fsx_extsize != 0) {
+            file_info->has_extent_size = true;
+            file_info->extent_size = attr.fsx_extsize;
+        }
+    }
+#endif
+
+    return spec_info;
+}
+
 static BlockStatsSpecificFile get_blockstats_specific_file(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
@@ -3301,6 +3329,7 @@ BlockDriver bdrv_file = {
     .bdrv_co_truncate = raw_co_truncate,
     .bdrv_getlength = raw_getlength,
     .bdrv_get_info = raw_get_info,
+    .bdrv_get_specific_info = raw_get_specific_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
     .bdrv_get_specific_stats = raw_get_specific_stats,
@@ -3673,6 +3702,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_co_truncate       = raw_co_truncate,
     .bdrv_getlength	= raw_getlength,
     .bdrv_get_info = raw_get_info,
+    .bdrv_get_specific_info = raw_get_specific_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
     .bdrv_get_specific_stats = hdev_get_specific_stats,
-- 
2.35.1

Re: [PATCH 1/4] block: Improve empty format-specific info dump

[Eric Blake]

On Tue, May 03, 2022 at 04:55:26PM +0200, Hanna Reitz wrote:
> When a block driver supports obtaining format-specific information, but
> that object only contains optional fields, it is possible that none of
> them are present, so that dump_qobject() (called by
> bdrv_image_info_specific_dump()) will not print anything.
> 
> The callers of bdrv_image_info_specific_dump() put a header above this
> information ("Format specific information:\n"), which will look strange
> when there is nothing below.  Modify bdrv_image_info_specific_dump() to
> print this header instead of its callers, and only if there is indeed
> something to be printed.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  include/block/qapi.h |  3 ++-
>  block/qapi.c         | 41 +++++++++++++++++++++++++++++++++++++----
>  qemu-io-cmds.c       |  4 ++--
>  3 files changed, 41 insertions(+), 7 deletions(-)
>

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/4] block: Add protocol-specific image info

[Eric Blake]

On Tue, May 03, 2022 at 04:55:27PM +0200, Hanna Reitz wrote:
> The ImageInfo object currently only contains (optional) format-specific
> image information.  However, perhaps the protocol node can provide some
> additional information, so add a new field presenting it.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  qapi/block-core.json |  6 +++++-
>  block/qapi.c         | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/4] block: Print protocol-specific information

[Eric Blake]

On Tue, May 03, 2022 at 04:55:28PM +0200, Hanna Reitz wrote:
> Make bdrv_image_info_dump() print protocol-specific information.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  block/qapi.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

(and now I should probably go write a patch to expose protocol
information for NBD...)

> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 293983cf82..169ea08f70 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -921,4 +921,9 @@ void bdrv_image_info_dump(ImageInfo *info)
>          bdrv_image_info_specific_dump(info->format_specific,
>                                        "Format specific information:\n");
>      }
> +
> +    if (info->has_protocol_specific) {
> +        bdrv_image_info_specific_dump(info->protocol_specific,
> +                                      "Protocol specific information:\n");
> +    }
>  }
> -- 
> 2.35.1
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 4/4] block/file: Add file-specific image info

[Eric Blake]

On Tue, May 03, 2022 at 04:55:29PM +0200, Hanna Reitz wrote:
> Add some (optional) information that the file driver can provide for
> image files, namely the extent size.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  qapi/block-core.json | 26 ++++++++++++++++++++++++--
>  block/file-posix.c   | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 

> +++ b/block/file-posix.c
> @@ -3068,6 +3068,34 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>      return 0;
>  }
>  
> +static ImageInfoSpecific *raw_get_specific_info(BlockDriverState *bs,
> +                                                Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    ImageInfoSpecificFile *file_info = g_new0(ImageInfoSpecificFile, 1);
> +    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
> +
> +    *spec_info = (ImageInfoSpecific){
> +        .type = IMAGE_INFO_SPECIFIC_KIND_FILE,
> +        .u.file.data = file_info,
> +    };
> +
> +#ifdef FS_IOC_FSGETXATTR
> +    {
> +        struct fsxattr attr;
> +        int ret;
> +
> +        ret = ioctl(s->fd, FS_IOC_FSGETXATTR, &attr);
> +        if (!ret && attr.fsx_extsize != 0) {
> +            file_info->has_extent_size = true;
> +            file_info->extent_size = attr.fsx_extsize;
> +        }
> +    }
> +#endif

Can/should we fall back to stat's st_blksize when the ioctl produces
nothing?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 4/4] block/file: Add file-specific image info

[Hanna Reitz]

On 03.05.22 20:50, Eric Blake wrote:
> On Tue, May 03, 2022 at 04:55:29PM +0200, Hanna Reitz wrote:
>> Add some (optional) information that the file driver can provide for
>> image files, namely the extent size.
>>
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>> ---
>>   qapi/block-core.json | 26 ++++++++++++++++++++++++--
>>   block/file-posix.c   | 30 ++++++++++++++++++++++++++++++
>>   2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> +++ b/block/file-posix.c
>> @@ -3068,6 +3068,34 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>>       return 0;
>>   }
>>   
>> +static ImageInfoSpecific *raw_get_specific_info(BlockDriverState *bs,
>> +                                                Error **errp)
>> +{
>> +    BDRVRawState *s = bs->opaque;
>> +    ImageInfoSpecificFile *file_info = g_new0(ImageInfoSpecificFile, 1);
>> +    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
>> +
>> +    *spec_info = (ImageInfoSpecific){
>> +        .type = IMAGE_INFO_SPECIFIC_KIND_FILE,
>> +        .u.file.data = file_info,
>> +    };
>> +
>> +#ifdef FS_IOC_FSGETXATTR
>> +    {
>> +        struct fsxattr attr;
>> +        int ret;
>> +
>> +        ret = ioctl(s->fd, FS_IOC_FSGETXATTR, &attr);
>> +        if (!ret && attr.fsx_extsize != 0) {
>> +            file_info->has_extent_size = true;
>> +            file_info->extent_size = attr.fsx_extsize;
>> +        }
>> +    }
>> +#endif
> Can/should we fall back to stat's st_blksize when the ioctl produces
> nothing?

I don’t think so, that’s a different value.  For example, by default, we 
use an extent-size-hint of 1 MB for new images (which is applied at 
least on XFS), but that doesn’t change st_blksize (which is 4096 here).  
So I’d only report the extent-size for filesystems that actually 
differentiate between the two.

Hanna

Re: [PATCH 1/4] block: Improve empty format-specific info dump

[Kevin Wolf]

Am 03.05.2022 um 16:55 hat Hanna Reitz geschrieben:
> When a block driver supports obtaining format-specific information, but
> that object only contains optional fields, it is possible that none of
> them are present, so that dump_qobject() (called by
> bdrv_image_info_specific_dump()) will not print anything.
> 
> The callers of bdrv_image_info_specific_dump() put a header above this
> information ("Format specific information:\n"), which will look strange
> when there is nothing below.  Modify bdrv_image_info_specific_dump() to
> print this header instead of its callers, and only if there is indeed
> something to be printed.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 2/4] block: Add protocol-specific image info

[Kevin Wolf]

Am 03.05.2022 um 16:55 hat Hanna Reitz geschrieben:
> The ImageInfo object currently only contains (optional) format-specific
> image information.  However, perhaps the protocol node can provide some
> additional information, so add a new field presenting it.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  qapi/block-core.json |  6 +++++-
>  block/qapi.c         | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index beeb91952a..e7d6c2e0cc 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -236,6 +236,9 @@
>  # @format-specific: structure supplying additional format-specific
>  #                   information (since 1.7)
>  #
> +# @protocol-specific: structure supplying additional protocol-specific
> +#                     information (since 7.1)
> +#
>  # Since: 1.3
>  #
>  ##
> @@ -246,7 +249,8 @@
>             '*backing-filename': 'str', '*full-backing-filename': 'str',
>             '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
>             '*backing-image': 'ImageInfo',
> -           '*format-specific': 'ImageInfoSpecific' } }
> +           '*format-specific': 'ImageInfoSpecific',
> +           '*protocol-specific': 'ImageInfoSpecific' } }

I'm not a fan of this one. It solves the problem for exactly one special
case (even if admittedly a common one) and leaves everything else as it
is. It is unclear what it produces in configurations that aren't the
simple one format node on top of one protocol node layout.

I would rather interpret 'format-specific' as 'driver-specific' and make
the ImageInfo for any child node accessible.

With rbd we already interpret it like a generic driver thing that is not
just for formats that because it implements .bdrv_get_specific_info even
though we didn't have a 'protocol-specific' yet.

Making other nodes has precedence, too. 'backing-image' is even in the
context of this hunk. VMDK exposes its extents the same way. So maybe
what we really want is a 'children' list with the ImageInfo of every
child node. And then qemu-img could go through all children and print
headings like "Driver specific information for file (#block123)". Then
filters like blkdebug could add their information and it would be
printed, too.

>  ##
>  # @ImageCheck:
> diff --git a/block/qapi.c b/block/qapi.c
> index 51202b470a..293983cf82 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -262,6 +262,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
>      int64_t size;
>      const char *backing_filename;
>      BlockDriverInfo bdi;
> +    BlockDriverState *protocol_bs;
>      int ret;
>      Error *err = NULL;
>      ImageInfo *info;
> @@ -303,6 +304,24 @@ void bdrv_query_image_info(BlockDriverState *bs,
>      }
>      info->has_format_specific = info->format_specific != NULL;
>  
> +    /* Try to look for an unambiguous protocol node */
> +    protocol_bs = bs;
> +    while (protocol_bs && !QLIST_EMPTY(&protocol_bs->children)) {
> +        protocol_bs = bdrv_primary_bs(protocol_bs);
> +    }

If bs is already a leaf node, this duplicates the information, which
looks weird:

    $ build/qemu-img info -f file ~/tmp/test.raw
    image: /home/kwolf/tmp/test.raw
    file format: file
    virtual size: 10 GiB (10737418240 bytes)
    disk size: 7.63 GiB
    Format specific information:
        extent size: 1048576
    Protocol specific information:
        extent size: 1048576

>
> +    if (protocol_bs) {
> +        /* Assert that this is a protocol node */
> +        assert(QLIST_EMPTY(&protocol_bs->children));
> +
> +        info->protocol_specific = bdrv_get_specific_info(protocol_bs, &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            qapi_free_ImageInfo(info);
> +            goto out;
> +        }
> +        info->has_protocol_specific = info->protocol_specific != NULL;
> +    }
> +
>      backing_filename = bs->backing_file;
>      if (backing_filename[0] != '\0') {
>          char *backing_filename2;

Kevin

Re: [PATCH 4/4] block/file: Add file-specific image info

[Kevin Wolf]

Am 03.05.2022 um 16:55 hat Hanna Reitz geschrieben:
> Add some (optional) information that the file driver can provide for
> image files, namely the extent size.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  qapi/block-core.json | 26 ++++++++++++++++++++++++--
>  block/file-posix.c   | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e7d6c2e0cc..728da051ae 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -139,16 +139,29 @@
>        '*encryption-format': 'RbdImageEncryptionFormat'
>    } }
>  
> +##
> +# @ImageInfoSpecificFile:
> +#
> +# @extent-size: Extent size (if available)
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'ImageInfoSpecificFile',
> +  'data': {
> +      '*extent-size': 'size'
> +  } }

It's not "the extent size" (the whole point of extents is that they
don't have a fixed size like blocks), but an extent size *hint* that
tells the filesystem the minimum size to allocate for an extent. The
xfs_io man page calls it the preferred extent size for allocatino, which
works for the documentation if you prefer, but BlockdevCreateOptionsFile
has 'extent-size-hint', so I'd prefer consistency on the wire at least.

Kevin

Re: [PATCH 2/4] block: Add protocol-specific image info

[Hanna Reitz]

On 04.05.22 10:36, Kevin Wolf wrote:
> Am 03.05.2022 um 16:55 hat Hanna Reitz geschrieben:
>> The ImageInfo object currently only contains (optional) format-specific
>> image information.  However, perhaps the protocol node can provide some
>> additional information, so add a new field presenting it.
>>
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>> ---
>>   qapi/block-core.json |  6 +++++-
>>   block/qapi.c         | 19 +++++++++++++++++++
>>   2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index beeb91952a..e7d6c2e0cc 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -236,6 +236,9 @@
>>   # @format-specific: structure supplying additional format-specific
>>   #                   information (since 1.7)
>>   #
>> +# @protocol-specific: structure supplying additional protocol-specific
>> +#                     information (since 7.1)
>> +#
>>   # Since: 1.3
>>   #
>>   ##
>> @@ -246,7 +249,8 @@
>>              '*backing-filename': 'str', '*full-backing-filename': 'str',
>>              '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
>>              '*backing-image': 'ImageInfo',
>> -           '*format-specific': 'ImageInfoSpecific' } }
>> +           '*format-specific': 'ImageInfoSpecific',
>> +           '*protocol-specific': 'ImageInfoSpecific' } }
> I'm not a fan of this one. It solves the problem for exactly one special
> case (even if admittedly a common one) and leaves everything else as it
> is. It is unclear what it produces in configurations that aren't the
> simple one format node on top of one protocol node layout.

I don’t disagree, but I do wonder how often this structure is used 
outside of `qemu-img info`, where filter nodes and more complex 
configurations are very rare.  I understand wanting to support complex 
block graph configurations everywhere, I’m just wondering whether there 
is actually much of a use for that here.

> I would rather interpret 'format-specific' as 'driver-specific' and make
> the ImageInfo for any child node accessible.

Again, I don’t disagree, but I have reservations about that.  I don’t 
think this is a trivial approach to take.

First, that will be kind of bad for VMDK files, which already have all 
of their extent children in their driver-specific info, so we’d 
duplicate that info.

Second, same for the backing child, generally.  Do we want to exclude 
specifically the backing child from that list of ImageInfos for all 
children, because we already have an entry for it in ImageInfo itself?  
That wouldn’t make much sense.  Deprecate backing-child?  Works for the 
future, weird in the meantime.

Third, the implementation would not be trivial. bdrv_query_image_info() 
specifically says to return "flat" image information, i.e. not to query 
the backing child information. Currently, its callers fill that blank 
some way or another, with `qemu-img info` creating a list of files (i.e. 
the backing chain) instead of using that backing-image field.  I 
actually have no idea how we should bring that together.  Should 
bdrv_query_image_info() also not collect that ImageInfo list of all 
children, and then collect_image_info_list() will put those into its 
list, too, making it recursive?  Then we have the problem of describing 
nodes in this graph, and as written below I wouldn’t be happy to use 
auto-generated node names for this.  Or should bdrv_query_image_info() 
collect all those children, and then collect_image_info_list() will just 
drop the backing child from them, so that it still gets a flat backing 
chain list, but the other children will be nested, allowing users to 
identify which nodes those are based on nesting?  (And nesting would 
require adding indentation support to bdrv_image_info_dump(), and 
bdrv_snapshot_dump().)

Fourth, precisely for the common case of not having filters or other 
more complex configurations, the additional info provided by the 
protocol node’s ImageInfo is limited.  Most of it just duplicates 
information from the format node, the really interesting bit is just the 
ImageInfoSpecific, so for `qemu-img info` it’ll mostly just clutter the 
output.  Many fields are also named on the assumption that this 
information is about a format node ("file format", "virtual size"), and 
so I personally find it confusing to see those things in the information 
about a protocol node when using `qemu-img info`.

> With rbd we already interpret it like a generic driver thing that is not
> just for formats that because it implements .bdrv_get_specific_info even
> though we didn't have a 'protocol-specific' yet.

On one hand, that’s the same thing I’m doing in this series.  On the 
other, I think the rbd implementation as a whole has not been well 
thought out, because it must have faced exactly the same problem that 
I’m trying to solve in this patch here, but obviously it hasn’t been 
addressed yet.

(Instead, it probably relied on users calling `qemu-img info -f rbd`, 
which is just cheating.  I mean, I could do that, too, and just drop 
anything but patch 4.)

> Making other nodes has precedence, too. 'backing-image' is even in the
> context of this hunk. VMDK exposes its extents the same way.

Both of which now make the solution to include the list of all 
children’s ImageInfos just more complicated, yes. O:)

(I know that me saying that simply means that these were probably bad 
solutions then, and that maybe we should’ve had a list of all children’s 
ImageInfos from the start.  Which means dancing around the issue even 
more won’t make it better, I know.  O:)  I’m just trying to say that 
simply adding this list isn’t an ideal solution now, under the current 
circumstances; but I’m not saying there is any ideal solution.)

> So maybe
> what we really want is a 'children' list with the ImageInfo of every
> child node. And then qemu-img could go through all children and print
> headings like "Driver specific information for file (#block123)".

I would very much rather drop auto-generated node names, and instead 
just print the child name and rely on indentation.  I have an example below.

> Then
> filters like blkdebug could add their information and it would be
> printed, too.

Is this really something that would ever be useful in practice?


I understand your concern (and share it to a degree), but I feel like 
allowing for this ImageInfo struct to represent and encompass a complex 
block graph comes at the detriment of readability and understandability 
of `qemu-img info` output for plain images.

For example, this is how I’d imagine the output for a raw image:

image: test.raw
file format: raw
virtual size: 10 GiB (10737418240 bytes)
disk size: 1 MiB
child 'file':
     image: test.raw
     file format: file
     virtual size: 10 GiB (10737418240 bytes)
     disk size: 1 MiB
     Driver specific information:
         extent size: 1048576

Personally, I like that less than what this series’s v1 produces.  I 
understand it represents the modular nature of the block graph, but 
that’s generally not something I want to see when I run `qemu-img info` 
on a plain image (which is 98 % of the use I have for `qemu-img info`).

>>   ##
>>   # @ImageCheck:
>> diff --git a/block/qapi.c b/block/qapi.c
>> index 51202b470a..293983cf82 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -262,6 +262,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
>>       int64_t size;
>>       const char *backing_filename;
>>       BlockDriverInfo bdi;
>> +    BlockDriverState *protocol_bs;
>>       int ret;
>>       Error *err = NULL;
>>       ImageInfo *info;
>> @@ -303,6 +304,24 @@ void bdrv_query_image_info(BlockDriverState *bs,
>>       }
>>       info->has_format_specific = info->format_specific != NULL;
>>   
>> +    /* Try to look for an unambiguous protocol node */
>> +    protocol_bs = bs;
>> +    while (protocol_bs && !QLIST_EMPTY(&protocol_bs->children)) {
>> +        protocol_bs = bdrv_primary_bs(protocol_bs);
>> +    }
> If bs is already a leaf node, this duplicates the information, which
> looks weird:
>
>      $ build/qemu-img info -f file ~/tmp/test.raw
>      image: /home/kwolf/tmp/test.raw
>      file format: file
>      virtual size: 10 GiB (10737418240 bytes)
>      disk size: 7.63 GiB
>      Format specific information:
>          extent size: 1048576
>      Protocol specific information:
>          extent size: 1048576

I mean, that isn’t wrong, but also fixable if need be.

>> +    if (protocol_bs) {
>> +        /* Assert that this is a protocol node */
>> +        assert(QLIST_EMPTY(&protocol_bs->children));
>> +
>> +        info->protocol_specific = bdrv_get_specific_info(protocol_bs, &err);
>> +        if (err) {
>> +            error_propagate(errp, err);
>> +            qapi_free_ImageInfo(info);
>> +            goto out;
>> +        }
>> +        info->has_protocol_specific = info->protocol_specific != NULL;
>> +    }
>> +
>>       backing_filename = bs->backing_file;
>>       if (backing_filename[0] != '\0') {
>>           char *backing_filename2;
> Kevin
>

Re: [PATCH 4/4] block/file: Add file-specific image info

[Hanna Reitz]

On 04.05.22 10:46, Kevin Wolf wrote:
> Am 03.05.2022 um 16:55 hat Hanna Reitz geschrieben:
>> Add some (optional) information that the file driver can provide for
>> image files, namely the extent size.
>>
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>> ---
>>   qapi/block-core.json | 26 ++++++++++++++++++++++++--
>>   block/file-posix.c   | 30 ++++++++++++++++++++++++++++++
>>   2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e7d6c2e0cc..728da051ae 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -139,16 +139,29 @@
>>         '*encryption-format': 'RbdImageEncryptionFormat'
>>     } }
>>   
>> +##
>> +# @ImageInfoSpecificFile:
>> +#
>> +# @extent-size: Extent size (if available)
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'ImageInfoSpecificFile',
>> +  'data': {
>> +      '*extent-size': 'size'
>> +  } }
> It's not "the extent size" (the whole point of extents is that they
> don't have a fixed size like blocks), but an extent size *hint* that
> tells the filesystem the minimum size to allocate for an extent. The
> xfs_io man page calls it the preferred extent size for allocatino, which
> works for the documentation if you prefer, but BlockdevCreateOptionsFile
> has 'extent-size-hint', so I'd prefer consistency on the wire at least.

Got it.