Re: [Qemu-devel] [PATCH 06/22] qcow2: add dirty bitmaps extension

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 01.10.2016 17:46, Max Reitz wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>> Add dirty bitmap extension as specified in docs/specs/qcow2.txt.
>> For now, just mirror extension header into Qcow2 state and check
>> constraints.
>>
>> For now, disable image resize if it has bitmaps. It will be fixed later.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.h |  4 +++
>>   2 files changed, 87 insertions(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c079aa8..08c4ef9 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
> [...]
>
>> @@ -162,6 +164,62 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>               }
>>               break;
>>   
>> +        case QCOW2_EXT_MAGIC_DIRTY_BITMAPS:
>> +            ret = bdrv_pread(bs->file, offset, &bitmaps_ext, ext.len);
> Overflows with ext.len > sizeof(bitmaps_ext).
>
> (ext.len < sizeof(bitmaps_ext) is also wrong, but less dramatically so.)
>
>> +            if (ret < 0) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Could not read ext header");
>> +                return ret;
>> +            }
>> +
>> +            if (bitmaps_ext.reserved32 != 0) {
>> +                error_setg_errno(errp, -ret, "ERROR: bitmaps_ext: "
>> +                                 "Reserved field is not zero.");
> Please drop the full stop at the end.

what do you mean? goto to fail: here? or not stop at all, just print error?

>
>> +                return -EINVAL;
>> +            }
>> +
>> +            be32_to_cpus(&bitmaps_ext.nb_bitmaps);
>> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_size);
>> +            be64_to_cpus(&bitmaps_ext.bitmap_directory_offset);
>> +
>> +            if (bitmaps_ext.nb_bitmaps > QCOW_MAX_DIRTY_BITMAPS) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too many dirty bitmaps");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.nb_bitmaps == 0) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "found bitmaps extension with zero bitmaps");
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.bitmap_directory_offset & (s->cluster_size - 1)) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "wrong dirty bitmap directory offset");
> s/wrong/invalid/
>
>> +                return -EINVAL;
>> +            }
>> +
>> +            if (bitmaps_ext.bitmap_directory_size >
>> +                QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
>> +                error_setg(errp, "ERROR: bitmaps_ext: "
>> +                                 "too large dirty bitmap directory");
>> +                return -EINVAL;
>> +            }
>> +
>> +            s->nb_bitmaps = bitmaps_ext.nb_bitmaps;
>> +            s->bitmap_directory_offset =
>> +                    bitmaps_ext.bitmap_directory_offset;
>> +            s->bitmap_directory_size =
>> +                    bitmaps_ext.bitmap_directory_size;
>> +
>> +#ifdef DEBUG_EXT
>> +            printf("Qcow2: Got dirty bitmaps extension:"
>> +                   " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
>> +                   s->bitmap_directory_offset, s->nb_bitmaps);
>> +#endif
>> +            break;
>> +
>>           default:
>>               /* unknown magic - save it in case we need to rewrite the header */
>>               {
> [...]
>
>> @@ -2509,6 +2585,13 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>>           return -ENOTSUP;
>>       }
>>   
>> +    /* cannot proceed if image has bitmaps */
>> +    if (s->nb_bitmaps) {
>> +        /* FIXME */
> I'd call it a TODO, but that's probably a matter of taste.
>
>> +        error_report("Can't resize an image which has bitmaps");
>> +        return -ENOTSUP;
>> +    }
>> +
>>       /* shrinking is currently not supported */
>>       if (offset < bs->total_sectors * 512) {
>>           error_report("qcow2 doesn't support shrinking images yet");
> Max
>


-- 
Best regards,
Vladimir