[libvirt] [PATCH v2 06/12] qemu: fix bugs in blockstats

[Peter Krempa]

On 12/16/14 14:17, John Ferlan wrote:
> 
> Ran the series through my local Coverity checker.... found one issue....
> 
> On 12/16/2014 03:04 AM, Eric Blake wrote:
>> The documentation for virDomainBlockInfo was confusing: it stated
>> that 'physical' was the size of the container, then gave an example
>> of it being the amount of storage used by a sparse file (that is,
>> for a sparse raw image on a regular file, the wording implied
>> capacity==physical, while allocation was smaller; but the example
>> instead claimed physical==allocation).  Since we use 'physical' for
>> the last offset of a block device, we should do likewise for
>> regular files.
>>
>> Furthermore, the example claimed that for a qcow2 regular file,
>> allocation==physical.  At the time the code was first written,
>> this was true (qcow2 files were allocated sequentially, and were
>> never sparse, so the last sector written happened to also match
>> the disk space occupied); but modern qemu does much better and
>> can punch holes for a qcow2 with allocation < physical.
>>
>> Basically, after this patch, the three fields are now reliably
>> mapped as:
>>  'capacity' - how much storage the guest can see (equal to
>> physical for raw images, determined by image metadata otherwise)
>>  'allocation' - how much storage the image occupies (similar to
>> what 'du' would report)
>>  'physical' - the last offset of the image (similar to what 'ls'
>> would report)
>>
>> 'capacity' can be larger than 'physical' (such as for a qcow2
>> image that does not vary much from a backing file) or smaller
>> (such as for a qcow2 file with lots of internal snapshots).
>> Likewise, 'allocation' can be (slightly) larger than 'physical'
>> (such as counting the tail of cluster allocations required to
>> round a file size up to filesystem granularity) or smaller
>> (for a sparse file).  A block-resize operation changes capacity
>> (which, for raw images, also changes physical); many non-raw
>> images automatically grow physical and allocation as necessary
>> when starting with an allocation smaller than capacity; and even
>> when capacity and physical stay unchanged, allocation can change
>> when converting sectors from holes to data or back.
>>
>> Note that this does not change semantics for qcow2 images stored
>> on block devices; there, we still rely on qemu to report the
>> highest written extent for allocation.  So using this API to
>> track when to extend a block device because a qcow2 image is
>> about to exceed a threshold will not see any changes.
>>
>> * include/libvirt/libvirt-domain.h (_virDomainBlockInfo): Tweak
>> documentation to match saner definition.
>> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): For regular
>> files, physical size is capacity, not allocation.
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---
>>  include/libvirt/libvirt-domain.h | 23 ++++++++++++++--------
>>  src/qemu/qemu_driver.c           | 42 ++++++++++++++++++++++------------------
>>  2 files changed, 38 insertions(+), 27 deletions(-)
>>

>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index f87c44b..5e9c133 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c

>> @@ -11145,16 +11149,16 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
>>                                                         buf, len)) < 0)
>>              goto endjob;
>>      }
>> -    if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
>> -                                                  format, NULL)))
>> +    if (format == VIR_STORAGE_FILE_RAW) {
>> +        disk->src->capacity = disk->src->physical;
>> +    } else if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf,
>> +                                                         len, format, NULL))) {
>>          goto endjob;
>> -    if (meta->capacity)
>> -        disk->src->capacity = meta->capacity;
>> +        disk->src->capacity = meta->capacity ? meta->capacity
>> +            : disk->src->physical;

Also if you are going to use a ternary, please leave all sections on one
line for clarity.

> 
> We'll never get to this line because of the goto endjob (which gets
> propagated in next patch as goto cleanup).
> 
> John
> 

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141216/cc454210/attachment-0001.sig>