[prev in list] [next in list] [prev in thread] [next in thread] 

List:       qemu-block
Subject:    Re: [Qemu-block] [PATCH for-4.2 08/13] qcow2: Fix broken snapshot table entries
From:       Max Reitz <mreitz () redhat ! com>
Date:       2019-07-31 9:06:52
Message-ID: f2fdcde0-d363-8e3d-e705-3931535c4060 () redhat ! com
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


On 30.07.19 21:02, Eric Blake wrote:
> On 7/30/19 12:25 PM, Max Reitz wrote:
>> The only case where we currently reject snapshot table entries is when
>> they have too much extra data.  Fix them with qemu-img check -r all by
>> counting it as a corruption, reducing their extra_data_size, and then
>> letting qcow2_check_fix_snapshot_table() do the rest.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2-snapshot.c | 69 ++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 56 insertions(+), 13 deletions(-)
>>
> 
>> @@ -112,16 +141,22 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
>>          }
>>  
>>          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);
>> -            if (ret < 0) {
>> -                error_setg_errno(errp, -ret, "Failed to read snapshot table");
>> -                goto fail;
>> +            if (discard_unknown_extra_data) {
>> +                /* Discard unknown extra data */
>> +                sn->extra_data_size = sizeof(extra);
> 
> This truncates it down to just the data we know. Should it instead
> truncate down to the 1024 bytes of QCOW_MAX_SNAPSHOT_EXTRA_DATA defined
> in 2/13?  (We can't keep all of the user's extra stuff, but we can at
> least try to preserve as much as possible)

On one hand, potentially cutting unknown data in half sounds like not
such a good idea to me.

On the other, a field can only be considered present if it is fully
present.  So cutting any optional data in half shouldn't have any
negative impact.

So, yes, truncating it down to 1024 bytes sounds good.

Max

> Otherwise, looks good.
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 



["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]