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

[John Ferlan]

On 09/27/2016 04:17 AM, Nikolay Shirokovskiy wrote:
> 
> 
> 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.

I agree... I don't have in depth knowledge of how the backing chains
work, so it's more of a question. Hopefully someone that understands
that logic (pkrempa? eblake?) would be able to be more definitive.

I just wouldn't want something to be missed if backing chains were required.

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

There's lots of similarities that I see in that code... I spent some
time today working through a mechanism to make one "query-block" call
that 3 consumers (BlockInfo, BlockStats, and DiskNameLookup) could use
(I have it working nominally at least). I would think this could then be
used to more easily add a new lookup function that's only returning the
data you want rather than putting stats data in an BlockInfo...

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

Note that I didn't say add a new param/return - I said make it more
multi-purpose with a subtly implied inference that perhaps a function
name change and a different return value (or local/static structure)
could work as well (and hence why I tried to combine things today to see
what was possible).

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

True, but I'm concerned that by adding an error path to existing code
there's some 'strange' path or device that previously wasn't causing an
error that now would be.  It's the paranoia of this code. I think eblake
has extensive knowledge of the QEMU side/layout.

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

Maybe it's my novice/naive knowledge of backing chains, but if you're
taking a backing and some data was in the backing-chain would it be lost
or does the API you're calling handle that for you.  Again, I don't have
a lot of exposure to that code. I do know there's still upstream qemu
work taking place. We really should be very careful about adding
anything to libvirt before the upstream work is done. In particular any
x-* command support.

John