qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_b


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 09/21] qcow2: add .bdrv_load_autoloading_dirty_bitmaps
Date: Fri, 16 Dec 2016 15:37:12 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1

On 14.12.2016 16:54, Vladimir Sementsov-Ogievskiy wrote:
> 07.12.2016 23:51, Max Reitz wrote:
>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
>>> Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
>>> are loaded when the image is opened and become BdrvDirtyBitmaps for the
>>> corresponding drive.
>>>
>>> Extra data in bitmaps is not supported for now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   block/Makefile.objs  |   2 +-
>>>   block/qcow2-bitmap.c | 663
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   block/qcow2.c        |   2 +
>>>   block/qcow2.h        |   3 +
>>>   4 files changed, 669 insertions(+), 1 deletion(-)
>>>   create mode 100644 block/qcow2-bitmap.c
>>>
>> [...]
>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> new file mode 100644
>>> index 0000000..0f797e6
>>> --- /dev/null
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -0,0 +1,663 @@
>> [...]
>>
>>> +/* Check table entry specification constraints. If cluster_size is
>>> 0, offset
>>> + * alignment is not checked. */
>>> +static int check_table_entry(uint64_t entry, int cluster_size)
>>> +{
>>> +    uint64_t offset;
>>> +
>>> +    if (entry & BME_TABLE_ENTRY_RESERVED_MASK) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
>>> +    if (offset != 0) {
>>> +        /* if offset specified, bit 0 must is reserved */
>> -must
>>
>>> +        if (entry & 1) {
>>> +            return -EINVAL;
>>> +        }
>>> +
>>> +        if ((cluster_size != 0) && (entry % cluster_size != 0)) {
>> Why would cluster_size be 0? Also, shouldn't it be offset instead of
>> entry?
> 
> the comment says: "If cluster_size is 0, offset alignment is not checked"

Oops, right. Is there any place where this function is called with
cluster_size being 0, though?

> yes, should be offset.
> 
>>
>>> +            return -EINVAL;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>> [...]
>>
>>> +/* bitmap table entries must satisfy specification constraints */
>>> +static int load_bitmap_data(BlockDriverState *bs,
>>> +                            const uint64_t *bitmap_table,
>>> +                            uint32_t bitmap_table_size,
>>> +                            BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    int ret = 0;
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    uint64_t sector, dsc;
>>> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
>>> +    uint8_t *buf = NULL;
>>> +    uint64_t i, tab_size =
>>> +            size_to_clusters(s,
>>> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0,
>>> bm_size));
>>> +
>>> +    if (tab_size != bitmap_table_size ||
>>> +            tab_size > BME_MAX_TABLE_SIZE) {
>> This line should be aligned to the opening parenthesis.
>>
>> [...]
>>
>>> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry
>>> *entry)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    uint64_t phys_bitmap_bytes =
>>> +            (uint64_t)entry->bitmap_table_size * s->cluster_size;
>>> +    uint64_t max_virtual_bits =
>>> +            (phys_bitmap_bytes * 8) << entry->granularity_bits;
>> I think this shift can overflow, even if bitmap_table_size, cluster_size
>> and granularity_bits are all within their respective limits.
> 
> Hmm. But it can't, if phys_bitmap_bytes <= MAX_PHYS_SIZE. I can just
> move this calculation down.

Yes, you're right, I missed the check on phys_bitmap_bytes. It's all
good then.

>>> +    int64_t nb_sectors = bdrv_nb_sectors(bs);
>> Just using bdrv_getlength() would probably be simpler.
>>
>>> +
>>> +    if (nb_sectors < 0) {
>>> +        return nb_sectors;
>>> +    }
>>> +
>>> +    int fail =
>> I'd prefer a boolean.
> 
> and mixed definition/code)

...now that you mention it... :-)

>>> +            (entry->bitmap_table_size == 0) ||
>>> +            (entry->bitmap_table_offset == 0) ||
>>> +            (entry->bitmap_table_offset % s->cluster_size) ||
>>> +            (entry->bitmap_table_size > BME_MAX_TABLE_SIZE) ||
>>> +            (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
>>> +            (entry->bitmap_table_offset != 0 &&
> 
> hmm, can be dropped, as checked above
> 
>>> +                (nb_sectors << BDRV_SECTOR_BITS) > max_virtual_bits) ||
>>> +            (entry->granularity_bits > BME_MAX_GRANULARITY_BITS) ||
>>> +            (entry->granularity_bits < BME_MIN_GRANULARITY_BITS) ||
>>> +            (entry->flags & BME_RESERVED_FLAGS) ||
>>> +            (entry->name_size > BME_MAX_NAME_SIZE) ||
>>> +            (entry->type != BT_DIRTY_TRACKING_BITMAP);
>>> +
>>> +    return fail ? -EINVAL : 0;
>>> +}
>> [...]
>>
>>> +/* bitmap_list_load
>>> + * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>>> + * checks it and convert to bitmap list.
>>> + */
>>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
>>> uint64_t offset,
>>> +                                         uint64_t size, Error **errp)
>>> +{
>> [...]
>>
>>> +    bm_list = bitmap_list_new();
>>> +    for (e = (Qcow2BitmapDirEntry *)dir;
>>> +             e < (Qcow2BitmapDirEntry *)dir_end; e =
>>> next_dir_entry(e)) {
>> This line should be aligned to the opening parenthesis.
>>
>> [...]
>>
>>> +
>>> +/* bitmap_list_store
>>> + * Store bitmap list to qcow2 image as a bitmap directory.
>>> + * Everything is checked.
>>> + */
>>> +static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList
>>> *bm_list,
>>> +                             uint64_t *offset, uint64_t *size, bool
>>> in_place)
>>> +{
>> [...]
>>
>>> +fail:
>>> +    g_free(dir);
>>> +
>>> +    if (dir_offset > 0) {
>>> +        qcow2_free_clusters(bs, dir_offset, dir_size,
>>> QCOW2_DISCARD_OTHER);
>> I think this should only be freed if in_place is false.
> 
> Reasonable.. And this also leads me to the think that we should clear
> autoclear bit in the header before inplace updating of bitmap directory
> and set it after successful update.

Not a bad idea, actually. I don't think it's absolutely necessary but it
would indeed be nice.

Max

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]