Re: [Qemu-devel] [PATCH 1/6] block/qapi: Add cache information to query-block

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/qapi.c               | 10 ++++++++++
>  hmp.c                      |  8 ++++++++
>  qapi/block-core.json       |  4 +++-
>  tests/qemu-iotests/051.out |  1 +
>  tests/qemu-iotests/067.out | 10 +++++-----
>  5 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 9733ebd..6b43bc3 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -46,6 +46,16 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState 
> *bs)
>      info->encrypted              = bs->encrypted;
>      info->encryption_key_missing = bdrv_key_required(bs);
>  
> +    info->cache = g_new(BlockdevCacheOptions, 1);
> +    *info->cache = (BlockdevCacheOptions) {
> +        .writeback      = bdrv_enable_write_cache(bs),
> +        .has_writeback  = true,
> +        .direct         = !!(bs->open_flags & BDRV_O_NOCACHE),
> +        .has_direct     = true,
> +        .no_flush       = !!(bs->open_flags & BDRV_O_NO_FLUSH),
> +        .has_no_flush   = true,
> +    };
> +

Awkward optionality.  I'm going to discuss this below, in the context of
the schema change.

>      if (bs->node_name[0]) {
>          info->has_node_name = true;
>          info->node_name = g_strdup(bs->node_name);
> diff --git a/hmp.c b/hmp.c
> index 40a90da..c3846b8 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -294,6 +294,7 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>  {
>      BlockInfoList *block_list, *info;
>      ImageInfo *image_info;
> +    BlockDeviceInfo *inserted;
>      const char *device = qdict_get_try_str(qdict, "device");
>      bool verbose = qdict_get_try_bool(qdict, "verbose", 0);
>  
> @@ -335,6 +336,13 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>              continue;
>          }
>  
> +        inserted = info->value->inserted;
> +
> +        monitor_printf(mon, "    Cache mode:       %s%s%s\n",
> +                       inserted->cache->writeback ? "writeback" : 
> "writethrough",
> +                       inserted->cache->direct ? ", direct" : "",
> +                       inserted->cache->no_flush ? ", ignore flushes" : "");
> +

If !inserted->cache->writeback && inserted->cache->direct, this prints
"    Cache mode:       , writeback".

>          if (info->value->inserted->has_backing_file) {
>              monitor_printf(mon,
>                             "    Backing file:     %s "
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 95dcd81..c53b587 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -238,6 +238,8 @@
>  #
>  # @iops_size: #optional an I/O size in bytes (Since 1.7)
>  #
> +# @cache: the cache mode used for the block device (since: 2.2)
> +#
>  # Since: 0.14.0
>  #
>  ##
> @@ -252,7 +254,7 @@
>              '*bps_max': 'int', '*bps_rd_max': 'int',
>              '*bps_wr_max': 'int', '*iops_max': 'int',
>              '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> -            '*iops_size': 'int' } }
> +            '*iops_size': 'int', 'cache': 'BlockdevCacheOptions' } }
>  
>  ##
>  # @BlockDeviceIoStatus:

Before this patch, QAPI type BlockdevCacheOptions is used only as a
member of BlockdevOptionsBase.

BlockdevOptionsBase is a collection configuration settings.
Consequently, it's a complex type whose members are optional exactly
when the configuration setting is optional.  Makes sense.

Complication: a few options are wrapped in another complex type,
BlockdevCacheOptions.  It's optional, but that's not sufficient, it's
members are all optional, too.

BlockdevCacheOptions is the only complex member of BlockdevOptionsBase.
The others are all simple or enum.

In this patch, you reuse BlockdevCacheOptions for a purpose other than
configuration: *querying* configuration.  In the query result, neither
the complex type nor its members are optional.  The schema reflects the
former (the patch hunk has 'cache', not '*cache'), but not the latter.

Do we want the schema to reflect reality accurately?

If no, we should still document the places where it doesn't, like this
one.

If yes, how can we fix this particular inaccuracy?

The obvious solution is not to reuse BlockdevCacheOptions.  Create a
second type, identical except for its members aren't optional.

Another idea is to add means to the schema to declare a member "deep
optional": not just the member is optional, but if it's present, its
members are optional, too.  Begs the question whether its member's
member's are optional.  Hmm.

Yet another idea is to declare nesting configuration options stupid, and
just not do it, but it may be too late for that.

Other ideas?

[...]