[libvirt] [PATCH 1/3] qemu: store guest visible disk size from qemu monitor block info

[John Ferlan]

On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote:
> ---
>  src/qemu/qemu_domain.h       |  1 +
>  src/qemu/qemu_monitor_json.c | 17 +++++++++++++++++
>  tests/qemumonitorjsontest.c  | 36 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 13c0372..ea57111 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -337,6 +337,7 @@ struct qemuDomainDiskInfo {
>      bool tray_open;
>      bool empty;
>      int io_status;
> +    unsigned long long guest_size;

a/k/a: 'virtual-size' in qemu and 'capacity' in libvirt

I probably would stick with capacity or something that says "disk"
rather than "guest".

>  };
>  

Trying to determine/decide whether this patch should be "separated" and
perhaps made usable with/for the existing callers or whether patch 3
should use the block stats (qemuDomainGetStatsBlock) which already
handles some details that could be missing here (at least w/r/t backing
chains).

Another option is adjusting the qemuMonitorJSONDiskNameLookup code to be
more multi-purpose since it's using the "query-block" for
*BlockPullCommon (from Rebase and Pull) as well as from *BlockCommit and
it already checks/expects both "inserted" and "image" to be present in
order to return that name.


>  typedef struct _qemuDomainHostdevPrivate qemuDomainHostdevPrivate;
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 3d0a120..7903b47 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1800,6 +1800,8 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
>  
>      for (i = 0; i < virJSONValueArraySize(devices); i++) {
>          virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
> +        virJSONValuePtr inserted;
> +        virJSONValuePtr image;
>          struct qemuDomainDiskInfo *info;
>          const char *thisdev;
>          const char *status;
> @@ -1854,6 +1856,21 @@ int qemuMonitorJSONGetBlockInfo(qemuMonitorPtr mon,
>              if (info->io_status < 0)
>                  goto cleanup;
>          }
> +
> +        if ((inserted = virJSONValueObjectGetObject(dev, "inserted"))) {
> +            if (!(image = virJSONValueObjectGetObject(inserted, "image"))) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("cannot read 'inserted/image' value"));
> +                goto cleanup;
> +            }

Other code that checks "inserted" and "image" doesn't fail - it just
ignores. If there's only going to be one consumer, then I don't believe
we want a failure such as this to affect other callers that may not
care. That could mean having some sort of "marker" in the returned
buffer that the fetch of "virtual-size" did occur (or just using not
zero). It would be a shame to have some unexpected failures for a field
that nothing else uses especially since the *GetBlockInfo is being used
during process launch and reconnect (via qemuProcessRefreshDisks).

Futhermore, what if there's a "backing-image" for "this" particular
path? Will that be important for the pull backups support? Without
looking at patch 3 I would think so...


John
> +
> +            if (virJSONValueObjectGetNumberUlong(image, "virtual-size",
> +                &info->guest_size) < 0) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("cannot read 'inserted/image/virtual-size' value"));
> +                goto cleanup;
> +            }
> +        }
>      }
>  
>      ret = 0;
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index d3ec3b1..d1922fd 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -50,6 +50,23 @@ const char *queryBlockReply =
>  "            \"locked\": false,"
>  "            \"removable\": false,"
>  "            \"inserted\": {"
> +"                \"image\": {"
> +"                    \"virtual-size\": 68719476736,"
> +"                    \"filename\": \"/home/zippy/work/tmp/gentoo.qcow2\","
> +"                    \"cluster-size\": 65536,"
> +"                    \"format\": \"qcow2\","
> +"                    \"actual-size\": 156901376,"
> +"                    \"format-specific\": {"
> +"                        \"type\": \"qcow2\","
> +"                        \"data\": {"
> +"                            \"compat\": \"1.1\","
> +"                            \"lazy-refcounts\": true,"
> +"                            \"refcount-bits\": 16,"
> +"                            \"corrupt\": false"
> +"                        }"
> +"                    },"
> +"                    \"dirty-flag\": false"
> +"                },"
>  "                \"iops_rd\": 5,"
>  "                \"iops_wr\": 6,"
>  "                \"ro\": false,"
> @@ -78,6 +95,13 @@ const char *queryBlockReply =
>  "            \"locked\": false,"
>  "            \"removable\": false,"
>  "            \"inserted\": {"
> +"                \"image\": {"
> +"                    \"virtual-size\": 34359738368,"
> +"                    \"filename\": \"/home/zippy/test.bin\","
> +"                    \"format\": \"raw\","
> +"                    \"actual-size\": 34359738368,"
> +"                    \"dirty-flag\": false"
> +"                },"
>  "                \"iops_rd\": 0,"
>  "                \"iops_wr\": 0,"
>  "                \"ro\": false,"
> @@ -99,6 +123,13 @@ const char *queryBlockReply =
>  "            \"locked\": true,"
>  "            \"removable\": true,"
>  "            \"inserted\": {"
> +"                \"image\": {"
> +"                    \"virtual-size\": 17179869184,"
> +"                    \"filename\": \"/home/zippy/test.bin\","
> +"                    \"format\": \"raw\","
> +"                    \"actual-size\": 17179869184,"
> +"                    \"dirty-flag\": false"
> +"                },"
>  "                \"iops_rd\": 0,"
>  "                \"iops_wr\": 0,"
>  "                \"ro\": true,"
> @@ -1413,6 +1444,8 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockInfo(const void *data)
>      if (VIR_ALLOC(info) < 0)
>          goto cleanup;
>  
> +    info->guest_size = 68719476736ULL;
> +
>      if (virHashAddEntry(expectedBlockDevices, "virtio-disk0", info) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         "Unable to create expectedBlockDevices hash table");
> @@ -1422,6 +1455,8 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockInfo(const void *data)
>      if (VIR_ALLOC(info) < 0)
>          goto cleanup;
>  
> +    info->guest_size = 34359738368ULL;
> +
>      if (virHashAddEntry(expectedBlockDevices, "virtio-disk1", info) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         "Unable to create expectedBlockDevices hash table");
> @@ -1434,6 +1469,7 @@ testQemuMonitorJSONqemuMonitorJSONGetBlockInfo(const void *data)
>      info->locked = true;
>      info->removable = true;
>      info->tray = true;
> +    info->guest_size = 17179869184ULL;
>  
>      if (virHashAddEntry(expectedBlockDevices, "ide0-1-0", info) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>