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@xxxxxxxxxx>
>> ---
>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list