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).

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c          | 11 +++++++++--
 tests/qemu-iotests/036 |  6 ++++--
 tests/qemu-iotests/061 |  6 ++++--
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 67b0c214fba4..9475ace57163 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2823,8 +2823,15 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }

-    /* 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.
+     */
+    if (s->qcow_version >= 3 && s->cluster_size > 4096) {
         static const Qcow2Feature features[] = {
             {
                 .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index 512598421c20..cf522de7a1aa 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -44,8 +44,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 # Only qcow2v3 and later supports feature bits;
-# qcow2.py does not support external data files
-_unsupported_imgopts 'compat=0.10' data_file
+# qcow2.py does not support external data files;
+# this test requires a cluster size large enough for the feature table
+_unsupported_imgopts 'compat=0.10' data_file \
+		     'cluster_size=\(512\|1024\|2048\|4096\)'

 echo
 echo === Image with unknown incompatible feature bit ===
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index 36b040491fef..ce285d308408 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -44,8 +44,10 @@ _supported_os Linux
 # Conversion between different compat versions can only really work
 # with refcount_bits=16;
 # we have explicit tests for data_file here, but the whole test does
-# not work with it
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
+# not work with it;
+# we have explicit tests for various cluster sizes, the remaining tests
+# require the default 64k cluster
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file cluster_size

 echo
 echo "=== Testing version downgrade with zero expansion ==="
-- 
2.26.0.rc2

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).

That’s a bit more brave than necessary, considering I don’t think anyone
has ever run the iotests with the cluster_size option.  (I certainly
don’t, and I don’t plan to, because I don’t think it’s that important to
test that.)  There are certainly many other iotests that fail with a
non-default cluster size.

Not that it’s wrong care about it.  On the opposite, I’m happy you do. :)

> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c          | 11 +++++++++--
>  tests/qemu-iotests/036 |  6 ++++--
>  tests/qemu-iotests/061 |  6 ++++--
>  3 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 67b0c214fba4..9475ace57163 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2823,8 +2823,15 @@ int qcow2_update_header(BlockDriverState *bs)
>          buflen -= ret;
>      }
> 
> -    /* 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) {
>          static const Qcow2Feature features[] = {
>              {
>                  .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,

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.  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?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

On 3/25/20 8:52 AM, Max Reitz wrote:

>> 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.

qemu enforces that the header is one cluster.  But you're right, that 
does not appear to directly be a limitation mandated by the spec, and we 
could relax qemu to allow the header to be several consecutive clusters. 
  The tricky part, however, is that the backing file name is NOT 
described by a header extension, but rather is just whatever bytes occur 
after the final header extension.  There's no clear indication anywhere 
on when to stop reading those bytes, other than by an implicit limit 
such as insisting those bytes fall within the first cluster.

Had we been smarter when designing v3, we would have made the backing 
file name a header extension (in fact, it would have been possible to 
design the additional fields of v3 to look like an unknown header 
extension when parsed by a v2 binary) - but hindsight is 20/20.

> 
> 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.

We'd still want a sane limit even for small-cluster images (maybe "no 
more than 2M of header information, regardless of cluster size); or 
maybe even introduce a NEW header field and/or extension to make it 
obvious how many clusters are being used for the purpose of the metadata 
header in this particular image (with sane fallbacks for when that 
extension is not present).  But you're right - it comes with complexity. 
  This patch as written is safe for 5.0-rc1, but this discussion about 
teaching qemu to permit headers larger than 1 cluster is squarely in the 
5.1 category, if at all.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org