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

[Nikolay Shirokovskiy]

On 26.09.2016 23:07, John Ferlan wrote:
> 
> 
> 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).

I donno, does backing chain changes anything in this case? I need virtual
size of top image only and anyway is it possible for images of backing
chain to have different virtual sizes? It does not make much sense.

qemuDomainGetStatsBlock (and more specifically qemuMonitorBlockStatsUpdateCapacity
which is more suited to get just size) has rather inconviniet arguments
more suited for many disks requests.

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

I would not make function with such specific name provide additionally size info...
I think it is perfectily fine to have many monitor commands that internally
use same 'query-block' qemu command because this command is really profound)

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

I doubt there can be 'inserted' without 'image', it doesn't make sense,
inserted what?)) I guess qemuDomainGetStatsBlock ignores missing 'image'
because it can afford it, while qemuMonitorJSONDiskNameLookup does not
really ignores, if name is not found then function return error eventually.
So if this is true, there should be no 'inserted' without 'image', then shame will go
to qemu)

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

Not really. Backing chain has nothing to do with backup, there is just no
difference from backup POV.


Nikolay