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

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(a)redhat.com&gt; > --- > 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;

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