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

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...