Re: [Qemu-devel] [PATCH for-4.2 09/13] qcow2: Fix overly long snapshot tables

[Eric Blake <eblake@redhat.com>]

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

On 7/30/19 12:25 PM, Max Reitz wrote:
> We currently refuse to open qcow2 images with overly long snapshot
> tables.  This patch makes qemu-img check -r all drop all offending
> entries past what we deem acceptable.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-snapshot.c | 89 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 79 insertions(+), 10 deletions(-)

I'm less sure about this one.  8/13 should have no semantic effect (if
the user _depended_ on that much extra data, they should have set an
incompatible feature flag bit, at which point we'd leave their data
alone because we don't recognize the feature bit; so it is safe to
assume the user did not depend on the data and that we can thus nuke it
with impunity).  But here, we are throwing away the user's internal
snapshots, and not even giving them a say in which ones to throw away
(more likely, by trimming from the end, we are destroying the most
recent snapshots in favor of the older ones - but I could argue that
throwing away the oldest also has its uses).


> @@ -417,7 +461,32 @@ int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
>  
>          return ret;
>      }
> -    result->corruptions += extra_data_dropped;
> +    result->corruptions += nb_clusters_reduced + extra_data_dropped;
> +
> +    if (nb_clusters_reduced) {
> +        /*
> +         * Update image header now, because:
> +         * (1) qcow2_check_refcounts() relies on s->nb_snapshots to be
> +         *     the same as what the image header says,
> +         * (2) this leaks clusters, but qcow2_check_refcounts() will
> +         *     fix that.
> +         */
> +        assert(fix & BDRV_FIX_ERRORS);
> +
> +        snapshot_table_pointer.nb_snapshots = cpu_to_be32(s->nb_snapshots);
> +        ret = bdrv_pwrite_sync(bs->file, 60,

That '60' needs a name; it keeps popping up.

If we like the patch, I didn't spot major coding problems.  But because
I'm not sure we want this patch, I'll skip R-b for now.

-- 
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 --]