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/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index ae2c49c..baef32d 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1356,7 +1356,7 @@ int virDomainBlockResize (virDomainPtr dom,
> /** virDomainBlockInfo:
> *
> * This struct provides information about the size of a block device
> - * backing store
> + * backing store.
> *
> * Examples:
> *
> @@ -1364,13 +1364,13 @@ int virDomainBlockResize (virDomainPtr dom,
> * * capacity, allocation, physical: All the same
> *
> * - Sparse raw file in filesystem:
> - * * capacity: logical size of the file
> - * * allocation, physical: number of blocks allocated to file
> + * * capacity, size: logical size of the file
> + * * allocation: disk space occupied by file
> *
> * - qcow2 file in filesystem
> * * capacity: logical size from qcow2 header
> - * * allocation, physical: logical size of the file /
> - * highest qcow extent (identical)
> + * * allocation: disk space occupied by file
> + * * physical: reported size of qcow2 file
> *
> * - qcow2 file in a block device
> * * capacity: logical size from qcow2 header
> @@ -1380,9 +1380,16 @@ int virDomainBlockResize (virDomainPtr dom,
> typedef struct _virDomainBlockInfo virDomainBlockInfo;
> typedef virDomainBlockInfo *virDomainBlockInfoPtr;
> struct _virDomainBlockInfo {
> - unsigned long long capacity; /* logical size in bytes of the block device backing image */
> - unsigned long long allocation; /* highest allocated extent in bytes of the block device backing image */
> - unsigned long long physical; /* physical size in bytes of the container of the backing image */
> + unsigned long long capacity; /* logical size in bytes of the
> + * image (how much storage the
> + * guest will see) */
> + unsigned long long allocation; /* host storage in bytes occupied
> + * by the image (such as highest
> + * allocated extent if there are no
> + * holes, similar to 'du') */
> + unsigned long long physical; /* host physical size in bytes of
> + * the image container (last
> + * offset, similar to 'ls')*/
> };
>
> int virDomainGetBlockInfo(virDomainPtr dom,
> 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
> @@ -11108,18 +11108,21 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
> /* Get info for normal formats */
> if (S_ISREG(sb.st_mode) || fd == -1) {
> #ifndef WIN32
> - disk->src->physical = (unsigned long long)sb.st_blocks *
> + disk->src->allocation = (unsigned long long)sb.st_blocks *
> (unsigned long long)DEV_BSIZE;
> #else
> + disk->src->allocation = sb.st_size;
> +#endif
> + /* Allocation tracks when the file is sparse, physical is the
> + * last offset of the file. */
> disk->src->physical = sb.st_size;
> -#endif
> - /* Regular files may be sparse, so logical size (capacity) is not same
> - * as actual physical above
> - */
> - disk->src->capacity = sb.st_size;
> } else {
> - /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
> - * be 64 bits on all platforms.
> + /* NB. Because we configure with AC_SYS_LARGEFILE, off_t
> + * should be 64 bits on all platforms. For block devices, we
> + * have to seek (safe even if someone else is writing) to
> + * determine physical size, and assume that allocation is the
> + * same as physical (but can refine that assumption later if
> + * qemu is still running).
> */
> end = lseek(fd, 0, SEEK_END);
> if (end == (off_t)-1) {
> @@ -11128,11 +11131,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
> goto endjob;
> }
> disk->src->physical = end;
> - disk->src->capacity = end;
> + disk->src->allocation = end;
> }
>
> - /* If the file we probed has a capacity set, then override
> - * what we calculated from file/block extents */
> + /* Raw files: capacity is physical size. For all other files: if
> + * the metadata has a capacity, use that, otherwise fall back to
> + * physical size. */
> if (!(format = virDomainDiskGetFormat(disk))) {
> if (!cfg->allowDiskFormatProbing) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -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
> + }
>
> - /* Set default value .. */
> - disk->src->allocation = disk->src->physical;
> -
> - /* ..but if guest is not using raw disk format and on a block device,
> + /* If guest is not using raw disk format and on a block device,
> * then query highest allocated extent from QEMU
> */
> if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK &&
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list