Re: [Qemu-devel] [PATCH v2] spec/qcow2: bitmaps: zero bitmap table offset

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

On 01.07.2016 11:12, Kevin Wolf wrote:
> Am 30.06.2016 um 19:23 hat Denis V. Lunev geschrieben:
>> On 06/30/2016 07:40 PM, John Snow wrote:
>>> On 06/30/2016 05:12 AM, Denis V. Lunev wrote:
>>>> On 06/30/2016 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> After loading bitmap from image and setting IN_USE flag in it's header,
>>>>> corresponding data (bitmap table and data clusters) becomes inconsistent
>>>>> and is no longer needed. It is better to free bitmap table and
>>>>> corresponding clusters from the image immediately after loading the
>>>>> bitmap than free them when the bitmap is saved, or deleted or set
>>>>> non-persistent.
>>>>>
>>>>> For now it is impossible to store only bitmap header without bitmap
>>>>> table, as specification requires it. Storing zeroed bitmap table (one or
>>>>> more clusters) is the only option to implement the behaviour similar to
>>>>> specified above.
>>>>>
>>>>> The same problem is for just storing empty bitmaps.
>>>>>
>>>>> This patch allows storing only bitmap header for empty bitmaps.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>
>>>>> Additional note. Should we also allow here bitmap_table_offset = 1, like
>>>>> in bitmap table, for the bitmap with all bits set? I am not sure that it
>>>>> is needed at all, but just to keep the company..
>>>>>
>>>>>    docs/specs/qcow2.txt | 5 ++++-
>>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>>>>> index 80cdfd0..9826222 100644
>>>>> --- a/docs/specs/qcow2.txt
>>>>> +++ b/docs/specs/qcow2.txt
>>>>> @@ -435,9 +435,12 @@ Structure of a bitmap directory entry:
>>>>>                        Offset into the image file at which the bitmap
>>>>> table
>>>>>                        (described below) for the bitmap starts. Must be
>>>>> aligned to
>>>>>                        a cluster boundary.
>>>>> +                    Zero value means that bitmap table is not
>>>>> allocated and the
>>>>> +                    bitmap should be considered as empty (all bits
>>>>> are zero).
>>>>>               8 - 11:    bitmap_table_size
>>>>> -                    Number of entries in the bitmap table of the bitmap.
>>>>> +                    Number of entries in the bitmap table of the
>>>>> bitmap. It
>>>>> +                    must be zero if bitmap_table_offset is zero.
>>>>>              12 - 15:    flags
>>>>>                        Bit
>>>> NACK
>>>>
>>>> no guys, we can not make this change at the moment.
>>>> We do have QEMU available in the field which is working
>>>> with the currently specified format.
>>>>
>>>> Den
>>>>
>>> But I think the new format is a /compatible/ change. Under the old spec,
>>> I think this field is *NEVER* zero.
>>>
>>> Am I wrong?
>> yes
>>
>> but as far as I can understand this breaks backward compatibility,
>> i.e. software working with the current revision of the specification
>> will not be able to load new images.
> This is okay if it means that it cleanly errors out. I'm a bit worried
> that it might take the 0 literally, though, and use the image header as
> the bitmap table. Can you check that, Den?
>
> If that's the case, we can make the change only with an incompatible
> feature bit, and that's probably not worth it.
>
> Kevin

Current version will error out with "Bitmap doesn't satisfy the 
constraints.", because I check that

int check_constraints(...)
   uint64_t phys_bitmap_bytes =
         (uint64_t)h->bitmap_table_size * cluster_size;
   uint64_t max_virtual_bits = (phys_bitmap_bytes * 8) << 
h->granularity_bits;
    int fail =
        .... ||
        (disk_size > max_virtual_bits) ||
         ...

     return fail ? -EINVAL : 0;
}

(max_virtual_bits will be zero if bitmap_table_size is zero)

-- 
Best regards,
Vladimir