[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header |
Date: |
Fri, 18 Oct 2019 13:54:18 +0000 |
18.10.2019 16:39, Eric Blake wrote:
> On 10/18/19 4:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>>> I would suggest a stronger requirement:
>>>>>
>>>>> header_length must be a multiple of 4, and must not land in the middle of
>>>>> any optional 8-byte field.
>>>>>
>>>>> Or maybe even add our compression type extension with 4 bytes of padding,
>>>>> so that we could go even stronger:
>>>>>
>>>>> header_length must be a multiple of 8.
>>>>
>>>> Hmm, if we imply that software will have to add some padding, than
>>>> requirement above about zero === feature-absence
>>>> becomes necessary. [*]
>>>>
>>>> Still I have two questions:
>>>> 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for
>>>> compression?
>
> No, fields can be smaller if that makes sense; but I still think it's
> important that fields don't straddle natural alignment boundaries. When
> adding just one field at a time, it's easier to add a wide field and less
> padding than a narrow field and lots of padding, if we're still striving for
> alignment.
>
>>>> 2. What is the benefit of padding, which you propose?
>
> The biggest benefit to keeping large fields from straddling alignment
> boundaries is that you can mmap() a qcow2 file and do naturally-aligned reads
> of those large fields, rather than byte-by-byte reads, without risking SIGBUS
> on some architectures. (You still have to worry about endianness, but that's
> true regardless of alignment)
>
>
>>> So, it looks inconsistent, if we pad all header extensions to 8 bytes
>>> except for the start of the first extension.
>>>
>>> I'll resend with padding soon.
>>
>>
>> Still, we have to make an exception at least for header_length = 104, which
>> is not a multiply of 8.
>
> Huh? 104 == 8 * 13, so our current v3 header size is 8-byte aligned.
> Likewise for 72 == 8 * 9 for v2 header size.
Ahahahaha, shame on my head:) I should go to school again.
>
>>
>> Also, is requiring alignment is an incompatible change of specification?
>
> No. I think it is just clarifying what was implicitly already the case.
> Remember, immediately after the header comes header extensions, and THOSE are
> explicitly required to be multiple-of-8 in size. That requirement makes no
> sense unless the header itself ends on an 8-byte alignment (which it does for
> all existing v2 and v3 images prior to our first official v3 header
> extension).
>
OK, let's go with it, I'll resend again.
--
Best regards,
Vladimir
[PATCH v7 2/2] docs: qcow2: introduce compression type feature, Vladimir Sementsov-Ogievskiy, 2019/10/07