[Qemu-devel] [RFC PATCH 0/3] Expand 'qemu-img info' to show protocol details

[Qemu-devel] [RFC PATCH 0/3] Expand 'qemu-img info' to show protocol details

[Eric Blake]

Posting this now to get feedback on the general idea; it would still
need to be polished into a form I'd feel happy committing, including
reworking 'qemu-nbd --list' to reuse the new NBD-specific QAPI type.

Based-on: <20190112175812.27068-1-eblake@redhat.com>
[0/19 nbd: add qemu-nbd --list]

Examples with this applied:
$ ./qemu-img info file
image: file
file format: qcow2
virtual size: 1.0M (1048576 bytes)
disk size: 672K
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false
Protocol specific information:
    write zero: true
    discard zero: true
    discard: true
    align: 1
$ qemu-nbd file &
$ ./qemu-img info nbd://localhost:10809
image: nbd://localhost:10809
file format: raw
virtual size: 1.0M (1048576 bytes)
disk size: unavailable
Protocol specific information:
    flags:
    active contexts:
        [0]:
            name: base:allocation
            id: 0
    unknown flags: 1260

Eric Blake (3):
  block: Expose protocol-specific data to 'qemu-img info'
  file: Expose some protocol-specific information
  RFC: nbd: Expose protocol-specific information

 qapi/block-core.json | 87 +++++++++++++++++++++++++++++++++++++++++++-
 block/nbd-client.h   |  1 +
 block/file-posix.c   | 21 +++++++++++
 block/nbd-client.c   | 39 ++++++++++++++++++++
 block/nbd.c          |  3 ++
 block/qapi.c         |  7 ++++
 6 files changed, 156 insertions(+), 2 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/3] block: Expose protocol-specific data to 'qemu-img info'

[Eric Blake]

'qemu-img info' is useful for showing additional information
about an image - but sometimes the interesting information is
specific to the protocol rather than to the format layer.  Set
the groundwork for showing this information; further patches
will then enable specific pieces of information.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-core.json | 6 +++++-
 block/qapi.c         | 7 +++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 91685be6c29..f28d5f5fc76 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -152,6 +152,9 @@
 # @format-specific: structure supplying additional format-specific
 # information (since 1.7)
 #
+# @protocol-specific: structure supplying additional protocol-specific
+# information (since 4.0)
+#
 # Since: 1.3
 #
 ##
@@ -162,7 +165,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 c66f949db83..a8d104ba8ce 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -284,6 +284,9 @@ void bdrv_query_image_info(BlockDriverState *bs,
     }
     info->format_specific     = bdrv_get_specific_info(bs);
     info->has_format_specific = info->format_specific != NULL;
+    info->protocol_specific =
+        bs->file ? bdrv_get_specific_info(bs->file->bs) : NULL;
+    info->has_protocol_specific = info->protocol_specific != NULL;

     backing_filename = bs->backing_file;
     if (backing_filename[0] != '\0') {
@@ -870,4 +873,8 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
         func_fprintf(f, "Format specific information:\n");
         bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
     }
+    if (info->has_protocol_specific) {
+        func_fprintf(f, "Protocol specific information:\n");
+        bdrv_image_info_specific_dump(func_fprintf, f, info->protocol_specific);
+    }
 }
-- 
2.20.1

[Qemu-devel] [PATCH 2/3] file: Expose some protocol-specific information

[Eric Blake]

When analyzing performance, it might be nice to let 'qemu-img info'
expose details that it learned from the underlying file or block
device.  Start the process by picking a few useful items determined
during raw_open_common().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-core.json | 24 +++++++++++++++++++++++-
 block/file-posix.c   | 21 +++++++++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index f28d5f5fc76..296c22a1003 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -101,6 +101,25 @@
       'extents': ['ImageInfo']
   } }

+##
+# @ImageInfoSpecificFile:
+#
+# Details about a file protocol BDS.
+#
+# @align: Alignment in use
+#
+# @discard: True if discard operations can be attempted
+#
+# @write-zero: True if efficient write zero operations can be attempted
+#
+# @discard-zero: True if discarding is known to read back as zero
+#
+# Since: 4.0
+##
+{ 'struct': 'ImageInfoSpecificFile',
+  'data': { 'align': 'int', 'discard': 'bool', 'write-zero': 'bool',
+            '*discard-zero': 'bool' } }
+
 ##
 # @ImageInfoSpecific:
 #
@@ -110,12 +129,15 @@
 ##
 { 'union': 'ImageInfoSpecific',
   'data': {
+      # Format drivers:
       'qcow2': 'ImageInfoSpecificQCow2',
       'vmdk': 'ImageInfoSpecificVmdk',
       # If we need to add block driver specific parameters for
       # LUKS in future, then we'll subclass QCryptoBlockInfoLUKS
       # to define a ImageInfoSpecificLUKS
-      'luks': 'QCryptoBlockInfoLUKS'
+      'luks': 'QCryptoBlockInfoLUKS',
+      # Protocol drivers:
+      'file': 'ImageInfoSpecificFile'
   } }

 ##
diff --git a/block/file-posix.c b/block/file-posix.c
index 8aee7a3fb8b..d4ce0e9116c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1079,6 +1079,23 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     bs->bl.opt_mem_alignment = MAX(s->buf_align, getpagesize());
 }

+static ImageInfoSpecific *raw_get_specific_info(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    ImageInfoSpecific *info = g_new0(ImageInfoSpecific, 1);
+    ImageInfoSpecificFile *infofile = g_new0(ImageInfoSpecificFile, 1);
+
+    info->type = IMAGE_INFO_SPECIFIC_KIND_FILE;
+    info->u.file.data = infofile;
+    infofile->align = s->buf_align;
+    infofile->discard = s->has_discard;
+    infofile->write_zero = s->has_write_zeroes;
+    infofile->has_discard_zero = s->has_discard;
+    infofile->discard_zero = s->discard_zeroes;
+
+    return info;
+}
+
 static int check_for_dasd(int fd)
 {
 #ifdef BIODASDINFO2
@@ -2765,6 +2782,7 @@ BlockDriver bdrv_file = {
     .bdrv_co_copy_range_from = raw_co_copy_range_from,
     .bdrv_co_copy_range_to  = raw_co_copy_range_to,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_get_specific_info = raw_get_specific_info,
     .bdrv_io_plug = raw_aio_plug,
     .bdrv_io_unplug = raw_aio_unplug,
     .bdrv_attach_aio_context = raw_aio_attach_aio_context,
@@ -3240,6 +3258,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_co_copy_range_from = raw_co_copy_range_from,
     .bdrv_co_copy_range_to  = raw_co_copy_range_to,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_get_specific_info = raw_get_specific_info,
     .bdrv_io_plug = raw_aio_plug,
     .bdrv_io_unplug = raw_aio_unplug,
     .bdrv_attach_aio_context = raw_aio_attach_aio_context,
@@ -3363,6 +3382,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_co_pwritev        = raw_co_pwritev,
     .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_get_specific_info = raw_get_specific_info,
     .bdrv_io_plug = raw_aio_plug,
     .bdrv_io_unplug = raw_aio_unplug,
     .bdrv_attach_aio_context = raw_aio_attach_aio_context,
@@ -3494,6 +3514,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_co_pwritev        = raw_co_pwritev,
     .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
     .bdrv_refresh_limits = raw_refresh_limits,
+    .bdrv_get_specific_info = raw_get_specific_info,
     .bdrv_io_plug = raw_aio_plug,
     .bdrv_io_unplug = raw_aio_unplug,
     .bdrv_attach_aio_context = raw_aio_attach_aio_context,
-- 
2.20.1

[Qemu-devel] [PATCH 3/3] RFC: nbd: Expose protocol-specific information

[Eric Blake]

Expose information that the NBD client remembers from its handshake
with the server. This is comparable to what 'qemu-nbd --list'
outputs when describing a server's capability, so a future patch
may refactor that to reuse this QAPI type.

The display of flags is interesting - rather than compute the
ImageInfoSpecificNBDFlagsList on every handshake, the client
code just saves raw flags. But it we know we will be presenting
things to an end user, it's nicer to have a list of flag names
than it is to have a raw integer that must be decoded.

Example output:
$ ./qemu-img info nbd://localhost:10809
image: nbd://localhost:10809
file format: raw
virtual size: 1.0M (1048576 bytes)
disk size: unavailable
Protocol specific information:
    flags:
    active contexts:
        [0]:
            name: base:allocation
            id: 0
    unknown flags: 1260

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

---
Work in progress; there are several todos that I would polish up if
the idea behind this patch is agreeable, including making the 'flags'
reporting accurate.  Also, if this works well, I'd like to improve
'qemu-nbd --list' to reuse this QAPI type.
---
 qapi/block-core.json | 59 +++++++++++++++++++++++++++++++++++++++++++-
 block/nbd-client.h   |  1 +
 block/nbd-client.c   | 39 +++++++++++++++++++++++++++++
 block/nbd.c          |  3 +++
 4 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 296c22a1003..f5fcd329dc6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -120,6 +120,62 @@
   'data': { 'align': 'int', 'discard': 'bool', 'write-zero': 'bool',
             '*discard-zero': 'bool' } }

+##
+# @ImageInfoSpecificNBDFlags:
+#
+# Enumeration of known NBD export flags.
+#
+# Since: 4.0
+##
+{ 'enum': 'ImageInfoSpecificNBDFlags', 'data': [
+    'readonly', 'flush', 'fua', 'rotational', 'trim', 'zeroes', 'df',
+    'multi', 'resize', 'cache'
+  ] }
+
+##
+# @ImageInfoSpecificNBDContext:
+#
+# Details about an NBD meta-context negotiated with NBD_OPT_SET_META_CONTEXT
+# and later observed via NBD_CMD_BLOCK_STATUS.
+#
+# @name: context name
+#
+# @id: context id, if context is active
+#
+# Since: 4.0
+##
+{ 'struct': 'ImageInfoSpecificNBDContext',
+  'data': { 'name': 'str', '*id': 'uint32' } }
+
+##
+# @ImageInfoSpecificNBD:
+#
+# @description: server's description of export
+#
+# @flags: listing of servers' export flags with names recognized by qemu
+#
+# @unknown-flags: any remaining export flags
+#
+# @min-block: minimum I/O size reported by server (assumed 512 if omitted)
+#
+# @opt-block: optimum I/O size reported by server
+#
+# @max-block: maximum I/O size reported by server (assumed 32M if omitted)
+#
+# @active-contexts: server meta-contexts in use by this client
+#
+# @contexts: server meta-contexts that were advertised but not selected
+#
+# Since: 4.0
+##
+{ 'struct': 'ImageInfoSpecificNBD',
+  'data': { '*description': 'str', '*flags': [ 'ImageInfoSpecificNBDFlags' ],
+            '*unknown-flags': 'uint16', '*min-block': 'uint32',
+            '*opt-block': 'uint32', '*max-block': 'uint32',
+            '*active-contexts': [ 'ImageInfoSpecificNBDContext' ],
+            '*contexts': [ 'ImageInfoSpecificNBDContext' ]
+  } }
+
 ##
 # @ImageInfoSpecific:
 #
@@ -137,7 +193,8 @@
       # to define a ImageInfoSpecificLUKS
       'luks': 'QCryptoBlockInfoLUKS',
       # Protocol drivers:
-      'file': 'ImageInfoSpecificFile'
+      'file': 'ImageInfoSpecificFile',
+      'nbd': 'ImageInfoSpecificNBD'
   } }

 ##
diff --git a/block/nbd-client.h b/block/nbd-client.h
index cfc90550b99..09f51ca2529 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -67,5 +67,6 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,
                                             int64_t offset, int64_t bytes,
                                             int64_t *pnum, int64_t *map,
                                             BlockDriverState **file);
+ImageInfoSpecific *nbd_client_specific_info(BlockDriverState *bs);

 #endif /* NBD_CLIENT_H */
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 813539676d2..0cc67d45b5e 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -952,6 +952,45 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,
            (extent.flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
 }

+ImageInfoSpecific *nbd_client_specific_info(BlockDriverState *bs)
+{
+    NBDClientSession *client = nbd_get_client_session(bs);
+    ImageInfoSpecific *info;
+    ImageInfoSpecificNBD *infonbd;
+
+    info = g_new0(ImageInfoSpecific, 1);
+    info->type = IMAGE_INFO_SPECIFIC_KIND_NBD;
+    infonbd = info->u.nbd.data = g_new0(ImageInfoSpecificNBD, 1);
+
+    /* TODO: Remember/expose server's description during handshake? */
+    if (client->info.flags & NBD_FLAG_HAS_FLAGS) {
+        uint16_t remaining = client->info.flags;
+
+        infonbd->has_flags = true;
+        remaining &= ~NBD_FLAG_HAS_FLAGS;
+        /* TODO: Populate flags */
+        if (remaining) {
+            infonbd->has_unknown_flags = true;
+            infonbd->unknown_flags = remaining;
+        }
+    }
+    /* TODO: Populate block sizing */
+    if (client->info.base_allocation) {
+        ImageInfoSpecificNBDContext *context;
+
+        infonbd->has_active_contexts = true;
+        infonbd->active_contexts = g_new0(ImageInfoSpecificNBDContextList, 1);
+        context = g_new(ImageInfoSpecificNBDContext, 1);
+        infonbd->active_contexts->value = context;
+        context->name = g_strdup(client->info.x_dirty_bitmap ?:
+                                 "base:allocation");
+        context->has_id = true;
+        context->id = client->info.context_id;
+    }
+    /* TODO: Remember/expose non-tracked contexts learned during handshake? */
+    return info;
+}
+
 void nbd_client_detach_aio_context(BlockDriverState *bs)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
diff --git a/block/nbd.c b/block/nbd.c
index e87699fb73b..8b6d3d6cf4a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -576,6 +576,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
+    .bdrv_get_specific_info     = nbd_client_specific_info,
     .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
@@ -596,6 +597,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
+    .bdrv_get_specific_info     = nbd_client_specific_info,
     .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
@@ -616,6 +618,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
+    .bdrv_get_specific_info     = nbd_client_specific_info,
     .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
-- 
2.20.1

Re: [Qemu-devel] [RFC PATCH 0/3] Expand 'qemu-img info' to show protocol details

[Daniel P. Berrangé]

On Thu, Jan 17, 2019 at 09:33:18AM -0600, Eric Blake wrote:
> Posting this now to get feedback on the general idea; it would still
> need to be polished into a form I'd feel happy committing, including
> reworking 'qemu-nbd --list' to reuse the new NBD-specific QAPI type.
> 
> Based-on: <20190112175812.27068-1-eblake@redhat.com>
> [0/19 nbd: add qemu-nbd --list]
> 
> Examples with this applied:
> $ ./qemu-img info file
> image: file
> file format: qcow2
> virtual size: 1.0M (1048576 bytes)
> disk size: 672K
> cluster_size: 65536
> Format specific information:
>     compat: 1.1
>     lazy refcounts: false
>     refcount bits: 16
>     corrupt: false
> Protocol specific information:
>     write zero: true
>     discard zero: true
>     discard: true
>     align: 1
> $ qemu-nbd file &
> $ ./qemu-img info nbd://localhost:10809
> image: nbd://localhost:10809
> file format: raw
> virtual size: 1.0M (1048576 bytes)
> disk size: unavailable
> Protocol specific information:
>     flags:
>     active contexts:
>         [0]:
>             name: base:allocation
>             id: 0
>     unknown flags: 1260

One thing that occurs to me is that the qemu-img info output data is
getting larger & larger over time. Especially with LUKS, the format
specific data is quite huge. Adding protocol specific info makes it
larger again.

In retrospect, IMHO, it would have been nice to not show the format
specific info by default, requiring a --verbose arg. Assuming we don't
want to change default behaviour though, we might as well show protocol
specific info by default too.

I wonder though if we could benefit from a '-b' / '--brief' arg to make
it switch to show only the short common metadata, for people who want a
more concise output.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC PATCH 0/3] Expand 'qemu-img info' to show protocol details

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

On 1/17/19 10:06 AM, Daniel P. Berrangé wrote:

> One thing that occurs to me is that the qemu-img info output data is
> getting larger & larger over time. Especially with LUKS, the format
> specific data is quite huge. Adding protocol specific info makes it
> larger again.
> 
> In retrospect, IMHO, it would have been nice to not show the format
> specific info by default, requiring a --verbose arg. Assuming we don't
> want to change default behaviour though, we might as well show protocol
> specific info by default too.
> 
> I wonder though if we could benefit from a '-b' / '--brief' arg to make
> it switch to show only the short common metadata, for people who want a
> more concise output.

Do we have any backwards-compatibility guarantees on the output?  The
--output=json is obviously going to be verbose, no matter what. But that
argues that any machine parsing should use --output=json, and that
--output=human can change without regards to back-compat.  Or, we could
have --output=brief as a new third ofmt option (without having to burn
-b), and/or make --output=full be the verbose human form, and let the
default --output=human stick to brief by default.


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 1/3] block: Expose protocol-specific data to 'qemu-img info'

[Kevin Wolf]

Am 17.01.2019 um 16:33 hat Eric Blake geschrieben:
> 'qemu-img info' is useful for showing additional information
> about an image - but sometimes the interesting information is
> specific to the protocol rather than to the format layer.  Set
> the groundwork for showing this information; further patches
> will then enable specific pieces of information.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Isn't this making invalid assumptions about the structure of the block
graph? Who says that I don't have a quorum node below the format layer
with five different protocol layer children? Some of which may have
filter nodes on top?

Kevin

Re: [Qemu-devel] [PATCH 1/3] block: Expose protocol-specific data to 'qemu-img info'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

On 1/17/19 10:39 AM, Kevin Wolf wrote:
> Am 17.01.2019 um 16:33 hat Eric Blake geschrieben:
>> 'qemu-img info' is useful for showing additional information
>> about an image - but sometimes the interesting information is
>> specific to the protocol rather than to the format layer.  Set
>> the groundwork for showing this information; further patches
>> will then enable specific pieces of information.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Isn't this making invalid assumptions about the structure of the block
> graph? Who says that I don't have a quorum node below the format layer
> with five different protocol layer children? Some of which may have
> filter nodes on top?

Does quorum have bs->file?  If not, then this field is not populated for
quorum, so it doesn't make invalid assumptions.

You do have a point about bs->file pointing to filters being an
interesting case, though.  Max had a series proposal back in August that
tries to do smarter role-based descent through the graph rather than
just hard-coding assumptions based on bs->file:

https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg01644.html

Maybe we need to get that in place first, at which point it then becomes
easier to reason about what related BDS information to visit in addition
to the format layer.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/3] file: Expose some protocol-specific information

[Vladimir Sementsov-Ogievskiy]

17.01.2019 18:33, Eric Blake wrote:
> When analyzing performance, it might be nice to let 'qemu-img info'
> expose details that it learned from the underlying file or block
> device.  Start the process by picking a few useful items determined
> during raw_open_common().
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   qapi/block-core.json | 24 +++++++++++++++++++++++-
>   block/file-posix.c   | 21 +++++++++++++++++++++
>   2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f28d5f5fc76..296c22a1003 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -101,6 +101,25 @@
>         'extents': ['ImageInfo']
>     } }
> 
> +##
> +# @ImageInfoSpecificFile:
> +#
> +# Details about a file protocol BDS.
> +#
> +# @align: Alignment in use
> +#
> +# @discard: True if discard operations can be attempted
> +#
> +# @write-zero: True if efficient write zero operations can be attempted
> +#
> +# @discard-zero: True if discarding is known to read back as zero
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'ImageInfoSpecificFile',
> +  'data': { 'align': 'int', 'discard': 'bool', 'write-zero': 'bool',
> +            '*discard-zero': 'bool' } }

Not sure if this works, as I remember raw-posix will adjust these fields
on first failed write, and without this first write, we will not actually
know are they supported.

> +
>   ##
>   # @ImageInfoSpecific:
>   #
> @@ -110,12 +129,15 @@
>   ##
>   { 'union': 'ImageInfoSpecific',
>     'data': {
> +      # Format drivers:
>         'qcow2': 'ImageInfoSpecificQCow2',
>         'vmdk': 'ImageInfoSpecificVmdk',
>         # If we need to add block driver specific parameters for
>         # LUKS in future, then we'll subclass QCryptoBlockInfoLUKS
>         # to define a ImageInfoSpecificLUKS
> -      'luks': 'QCryptoBlockInfoLUKS'
> +      'luks': 'QCryptoBlockInfoLUKS',
> +      # Protocol drivers:
> +      'file': 'ImageInfoSpecificFile'
>     } }
> 
>   ##
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 8aee7a3fb8b..d4ce0e9116c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1079,6 +1079,23 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>       bs->bl.opt_mem_alignment = MAX(s->buf_align, getpagesize());
>   }
> 
> +static ImageInfoSpecific *raw_get_specific_info(BlockDriverState *bs)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    ImageInfoSpecific *info = g_new0(ImageInfoSpecific, 1);
> +    ImageInfoSpecificFile *infofile = g_new0(ImageInfoSpecificFile, 1);
> +
> +    info->type = IMAGE_INFO_SPECIFIC_KIND_FILE;
> +    info->u.file.data = infofile;
> +    infofile->align = s->buf_align;
> +    infofile->discard = s->has_discard;
> +    infofile->write_zero = s->has_write_zeroes;
> +    infofile->has_discard_zero = s->has_discard;
> +    infofile->discard_zero = s->discard_zeroes;
> +
> +    return info;
> +}
> +
>   static int check_for_dasd(int fd)
>   {
>   #ifdef BIODASDINFO2
> @@ -2765,6 +2782,7 @@ BlockDriver bdrv_file = {
>       .bdrv_co_copy_range_from = raw_co_copy_range_from,
>       .bdrv_co_copy_range_to  = raw_co_copy_range_to,
>       .bdrv_refresh_limits = raw_refresh_limits,
> +    .bdrv_get_specific_info = raw_get_specific_info,
>       .bdrv_io_plug = raw_aio_plug,
>       .bdrv_io_unplug = raw_aio_unplug,
>       .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> @@ -3240,6 +3258,7 @@ static BlockDriver bdrv_host_device = {
>       .bdrv_co_copy_range_from = raw_co_copy_range_from,
>       .bdrv_co_copy_range_to  = raw_co_copy_range_to,
>       .bdrv_refresh_limits = raw_refresh_limits,
> +    .bdrv_get_specific_info = raw_get_specific_info,
>       .bdrv_io_plug = raw_aio_plug,
>       .bdrv_io_unplug = raw_aio_unplug,
>       .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> @@ -3363,6 +3382,7 @@ static BlockDriver bdrv_host_cdrom = {
>       .bdrv_co_pwritev        = raw_co_pwritev,
>       .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
>       .bdrv_refresh_limits = raw_refresh_limits,
> +    .bdrv_get_specific_info = raw_get_specific_info,
>       .bdrv_io_plug = raw_aio_plug,
>       .bdrv_io_unplug = raw_aio_unplug,
>       .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> @@ -3494,6 +3514,7 @@ static BlockDriver bdrv_host_cdrom = {
>       .bdrv_co_pwritev        = raw_co_pwritev,
>       .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,
>       .bdrv_refresh_limits = raw_refresh_limits,
> +    .bdrv_get_specific_info = raw_get_specific_info,
>       .bdrv_io_plug = raw_aio_plug,
>       .bdrv_io_unplug = raw_aio_unplug,
>       .bdrv_attach_aio_context = raw_aio_attach_aio_context,
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/3] block: Expose protocol-specific data to 'qemu-img info'

[Vladimir Sementsov-Ogievskiy]

17.01.2019 18:33, Eric Blake wrote:
> 'qemu-img info' is useful for showing additional information
> about an image - but sometimes the interesting information is
> specific to the protocol rather than to the format layer.  Set
> the groundwork for showing this information; further patches
> will then enable specific pieces of information.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   qapi/block-core.json | 6 +++++-
>   block/qapi.c         | 7 +++++++
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 91685be6c29..f28d5f5fc76 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -152,6 +152,9 @@
>   # @format-specific: structure supplying additional format-specific
>   # information (since 1.7)
>   #
> +# @protocol-specific: structure supplying additional protocol-specific
> +# information (since 4.0)
> +#
>   # Since: 1.3
>   #
>   ##
> @@ -162,7 +165,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 c66f949db83..a8d104ba8ce 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -284,6 +284,9 @@ void bdrv_query_image_info(BlockDriverState *bs,
>       }
>       info->format_specific     = bdrv_get_specific_info(bs);
>       info->has_format_specific = info->format_specific != NULL;
> +    info->protocol_specific =
> +        bs->file ? bdrv_get_specific_info(bs->file->bs) : NULL;
> +    info->has_protocol_specific = info->protocol_specific != NULL;
> 
>       backing_filename = bs->backing_file;
>       if (backing_filename[0] != '\0') {
> @@ -870,4 +873,8 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>           func_fprintf(f, "Format specific information:\n");
>           bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>       }
> +    if (info->has_protocol_specific) {
> +        func_fprintf(f, "Protocol specific information:\n");
> +        bdrv_image_info_specific_dump(func_fprintf, f, info->protocol_specific);
> +    }
>   }
> 


A bit related question:

Why do we always have nbd node as a protocol node? Can't we use nbd node directly,
without raw layer?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/3] file: Expose some protocol-specific information

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

On 1/18/19 8:08 AM, Vladimir Sementsov-Ogievskiy wrote:

>>
>> +##
>> +# @ImageInfoSpecificFile:
>> +#
>> +# Details about a file protocol BDS.
>> +#
>> +# @align: Alignment in use
>> +#
>> +# @discard: True if discard operations can be attempted
>> +#
>> +# @write-zero: True if efficient write zero operations can be attempted
>> +#
>> +# @discard-zero: True if discarding is known to read back as zero
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'ImageInfoSpecificFile',
>> +  'data': { 'align': 'int', 'discard': 'bool', 'write-zero': 'bool',
>> +            '*discard-zero': 'bool' } }
> 
> Not sure if this works, as I remember raw-posix will adjust these fields
> on first failed write, and without this first write, we will not actually
> know are they supported.

Indeed. We could make them a tri-state enum instead of a bool (unknown,
supported, unsupported) - but it would require code tweaks to use the
tri-state in the code too (basically, treat unknown and supported as a
request to try on the first write that needs it; and change unknown to
supported or unsupported after that first write).  Then again, I worded
it as "can be attempted", not "known to work", so while it is indeed
non-helpful for 'qemu-img info' (because the attempt wasn't resolved
into something known for sure), it DOES help a long-running qemu process
when querying the same information over QMP (where such writes have
probably been attempted in the past).

Again, this series is more of an RFC on what do we really want to expose
to the user, and when; and not necessarily a hard commitment that this
is the best struct.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]