Re: [Qemu-devel] [PATCH for-4.2 02/13] qcow2: Keep unknown extra snapshot data

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6866 bytes --]

On 7/30/19 12:24 PM, Max Reitz wrote:
> The qcow2 specification says to ignore unknown extra data fields in
> snapshot table entries.  Currently, we discard it whenever we update the
> image, which is a bit different from "ignore".
> 
> This patch makes the qcow2 driver keep all unknown extra data fields
> when updating an image's snapshot table.

The cover letter questioned whether we want this, but I think we do.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.h          |  5 ++++
>  block/qcow2-snapshot.c | 59 +++++++++++++++++++++++++++++++++++-------
>  2 files changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 175708cee0..290a48b77e 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -61,6 +61,9 @@
>   * space for snapshot names and IDs */
>  #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
>  
> +/* Maximum amount of extra data per snapshot table entry to accept */
> +#define QCOW_MAX_SNAPSHOT_EXTRA_DATA 1024
> +
>  /* Bitmap header extension constraints */
>  #define QCOW2_MAX_BITMAPS 65535
>  #define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
> @@ -178,6 +181,8 @@ typedef struct QCowSnapshot {
>      uint32_t date_sec;
>      uint32_t date_nsec;
>      uint64_t vm_clock_nsec;
> +    uint32_t extra_data_size;
> +    void *unknown_extra_data; /* Extra data past QCowSnapshotExtraData */

Is char* going to be any easier to use than void*?

> +++ b/block/qcow2-snapshot.c

> @@ -80,30 +80,52 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)

> +
> +        /* Read known extra data */
>          ret = bdrv_pread(bs->file, offset, &extra,
> -                         MIN(sizeof(extra), extra_data_size));
> +                         MIN(sizeof(extra), sn->extra_data_size));
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Failed to read snapshot table");
>              goto fail;
>          }
> -        offset += extra_data_size;
> +        offset += MIN(sizeof(extra), sn->extra_data_size);
>  
> -        if (extra_data_size >= 8) {
> +        if (sn->extra_data_size >= 8) {

While touching this, is it worth spelling it:
if (sn->extra_data_size >= sizeof(extra.vm_state_size_large)) {

>              sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
>          }
>  
> -        if (extra_data_size >= 16) {
> +        if (sn->extra_data_size >= 16) {

and a similar use of sizeof() instead of hard-coded 16 here?

>              sn->disk_size = be64_to_cpu(extra.disk_size);
>          } else {
>              sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>          }
>  
> +        if (sn->extra_data_size > sizeof(extra)) {
> +            /* Store unknown extra data */
> +            size_t unknown_extra_data_size =
> +                sn->extra_data_size - sizeof(extra);
> +
> +            sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
> +            ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
> +                             unknown_extra_data_size);

We're doing two separate bdrv_pread()s. Would it be better to do a
single bdrv_preadv into a vector composed of &extra and
&unknown_extra_data, for less I/O?  (Then again, this micro-optimization
is probably in the noise in the long run)

> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "Failed to read snapshot table");
> +                goto fail;
> +            }
> +            offset += unknown_extra_data_size;
> +        }
> +
>          /* Read snapshot ID */
>          sn->id_str = g_malloc(id_str_size + 1);
>          ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
> @@ -161,7 +183,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>          sn = s->snapshots + i;
>          offset = ROUND_UP(offset, 8);
>          offset += sizeof(h);
> -        offset += sizeof(extra);
> +        offset += MAX(sizeof(extra), sn->extra_data_size);
>          offset += strlen(sn->id_str);
>          offset += strlen(sn->name);
>  
> @@ -208,7 +230,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>          h.date_sec = cpu_to_be32(sn->date_sec);
>          h.date_nsec = cpu_to_be32(sn->date_nsec);
>          h.vm_clock_nsec = cpu_to_be64(sn->vm_clock_nsec);
> -        h.extra_data_size = cpu_to_be32(sizeof(extra));
> +        h.extra_data_size = cpu_to_be32(MAX(sizeof(extra),
> +                                            sn->extra_data_size));
>  
>          memset(&extra, 0, sizeof(extra));
>          extra.vm_state_size_large = cpu_to_be64(sn->vm_state_size);
> @@ -233,6 +256,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>          }
>          offset += sizeof(extra);
>  
> +        if (sn->extra_data_size > sizeof(extra)) {
> +            size_t unknown_extra_data_size =
> +                sn->extra_data_size - sizeof(extra);
> +
> +            /* qcow2_read_snapshots() ensures no unbounded allocation */
> +            assert(unknown_extra_data_size <= INT_MAX);

Should we make this tighter:
 assert(unknown_extra_data_size < QCOW_MAX_SNAPSHOT_EXTRA_DATA);
or even
 assert(unknown_extra_data_size <=
        QCOW_MAX_SNAPSHOT_EXTRA_DATA - sizeof(extra));

> +            assert(sn->unknown_extra_data);
> +
> +            ret = bdrv_pwrite(bs->file, offset, sn->unknown_extra_data,
> +                              unknown_extra_data_size);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +            offset += unknown_extra_data_size;
> +        }
> +
>          ret = bdrv_pwrite(bs->file, offset, sn->id_str, id_str_size);
>          if (ret < 0) {
>              goto fail;
> @@ -375,6 +414,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>      sn->date_sec = sn_info->date_sec;
>      sn->date_nsec = sn_info->date_nsec;
>      sn->vm_clock_nsec = sn_info->vm_clock_nsec;
> +    sn->extra_data_size = sizeof(QCowSnapshotExtraData);
>  
>      /* Allocate the L1 table of the snapshot and copy the current one there. */
>      l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
> @@ -646,6 +686,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
>       * The snapshot is now unused, clean up. If we fail after this point, we
>       * won't recover but just leak clusters.
>       */
> +    g_free(sn.unknown_extra_data);
>      g_free(sn.id_str);
>      g_free(sn.name);
>  
> 

Tweaks to assertions are minor enough that I'm okay with:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]