On 25.03.20 14:18, Eric Blake wrote:
> On 3/25/20 7:42 AM, Max Reitz wrote:
>> On 24.03.20 18:42, Eric Blake wrote:
>>> As the feature name table can be quite large (over 9k if all 64 bits
>>> of all three feature fields have names; a mere 8 features leaves only
>>> 8 bytes for a backing file name in a 512-byte cluster), it is unwise
>>> to emit this optional header in images with small cluster sizes.
>>>
>>> Update iotest 036 to skip running on small cluster sizes; meanwhile,
>>> note that iotest 061 never passed on alternative cluster sizes
>>> (however, I limited this patch to tests with output affected by adding
>>> feature names, rather than auditing for other tests that are not
>>> robust to alternative cluster sizes).
>>
> 
>>> -    /* Feature table */
>>> -    if (s->qcow_version >= 3) {
>>> +    /*
>>> +     * Feature table.  A mere 8 feature names occupies 392 bytes, and
>>> +     * when coupled with the v3 minimum header of 104 bytes plus the
>>> +     * 8-byte end-of-extension marker, that would leave only 8 bytes
>>> +     * for a backing file name in an image with 512-byte clusters.
>>> +     * Thus, we choose to omit this header for cluster sizes 4k and
>>> +     * smaller.
>>
>> Can’t we do this decision more dynamically?  Like, always omit it when
>> cluster_size - sizeof(features) < 2k/3k/...?
>>
>> Max
>>
>>> +     */
>>> +    if (s->qcow_version >= 3 && s->cluster_size > 4096) {
> 
> Yes.  But when you consider that sizeof(features) is a compile-time
> constant, it isn't really dynamic for a given qemu release, but rather a
> different way to spell things; about the only thing it would buy us is
> that our cutoff window for what cluster size no longer gets the header
> may automatically shift from 2k to 4k to 8k as future qemu versions add
> more qcow2 features.

Yes.

> If we want to write it like that, which size limit
> do you propose?  Or asked differently, how much space should we reserve
> for other extension headers + backing file name?

Well, that was the “2k/3k/...” list. :)

The backing file name is limited to 1k, so I suppose that plus 1k for a
potential external data filename, plus 1k for the rest, so 3k in total?

Now that I look into the spec, I see that it actually doesn’t say that
the backing filename has to be part of the header cluster.  But, well.

It also only says that the image header must be part of the first
cluster, which in my opinion doesn’t necessarily include its extensions.
 So header extensions (and the backing filename) could actually be in
consecutive clusters.  But that of course would be much more difficult
to implement.

Max