[libvirt PATCH 2/2] storage_file: add support to probe cluster_size from QCOW2 images

[Peter Krempa]

On Thu, May 13, 2021 at 13:23:05 +0200, Pavel Hrdina wrote:
> >From QEMU docs/interop/qcow2.txt :
> 
>    Byte  20 - 23:   cluster_bits
>                     Number of bits that are used for addressing an offset
>                     within a cluster (1 << cluster_bits is the cluster size).
> 
> With this patch libvirt will be able to report the current cluster_size
> for all existing storage volumes managed by storage driver.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/storage/storage_util.c            |  3 ++
>  src/storage_file/storage_file_probe.c | 55 +++++++++++++++++++--------
>  2 files changed, 42 insertions(+), 16 deletions(-)

[...]

> diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c
> index afe64da02e..423597049f 100644
> --- a/src/storage_file/storage_file_probe.c
> +++ b/src/storage_file/storage_file_probe.c
> @@ -94,6 +94,9 @@ struct FileTypeInfo {
>                            /* Store a COW base image path (possibly relative),
>                             * or NULL if there is no COW base image, to RES;
>                             * return BACKING_STORE_* */
> +    int clusterBitsOffset; /* Byte offset from start of file where we find
> +                            * number of cluster bits, -1 to skip. */
> +    int clusterBitsSize;   /* Number of bytes for cluster bits. */

This isn't actually used [1].

>      const struct FileEncryptionInfo *cryptInfo; /* Encryption info */
>      int (*getBackingStore)(char **res, int *format,
>                             const char *buf, size_t buf_size);

Given how the value is stored in the header it seems that it won't
really be usable for any other format. I think we should parse it via a
callback rather than the generic offset/size parser.

Specifically since the cluster size is stored as number of bits.

> @@ -116,7 +119,8 @@ qedGetBackingStore(char **, int *, const char *, size_t);
>  #define QCOWX_HDR_VERSION (4)
>  #define QCOWX_HDR_BACKING_FILE_OFFSET (QCOWX_HDR_VERSION+4)
>  #define QCOWX_HDR_BACKING_FILE_SIZE (QCOWX_HDR_BACKING_FILE_OFFSET+8)
> -#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_BACKING_FILE_SIZE+4+4)
> +#define QCOWX_HDR_CLUSTER_BITS_OFFSET (QCOWX_HDR_BACKING_FILE_SIZE+4)
> +#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_CLUSTER_BITS_OFFSET+4)
>  
>  #define QCOW1_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8+1+1+2)
>  #define QCOW2_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8)



>  G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST);
> @@ -890,6 +896,23 @@ virStorageFileProbeGetMetadata(virStorageSource *meta,
>          meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier;
>      }
>  
> +    if (fileTypeInfo[meta->format].clusterBitsOffset != -1) {
> +        int clusterBits = 0;
> +
> +        if ((fileTypeInfo[meta->format].clusterBitsOffset + 4) > len)

[1] ... you hardcode the value.

> +            return 0;
> +
> +        if (fileTypeInfo[meta->format].endian == LV_LITTLE_ENDIAN)
> +            clusterBits = virReadBufInt32LE(buf +
> +                                            fileTypeInfo[meta->format].clusterBitsOffset);
> +        else
> +            clusterBits = virReadBufInt32BE(buf +
> +                                            fileTypeInfo[meta->format].clusterBitsOffset);
> +
> +        if (clusterBits > 0)
> +            meta->clusterSize = 1 << clusterBits;
> +    }
> +
>      VIR_FREE(meta->backingStoreRaw);
>      if (fileTypeInfo[meta->format].getBackingStore != NULL) {
>          int store = fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw,
> -- 
> 2.31.1
>