[Qemu-devel] [PATCH] qcow2: Clarify that compressed cluster offset requires shift

[Qemu-devel] [PATCH] qcow2: Clarify that compressed cluster offset requires shift

[Eric Blake]

The specs for the host cluster offset of a compressed cluster
were not clear that the offset is in terms of sectors, and requires
a shift by 9 to be a byte offset.  Add some more text to make the
interpretation obvious.

CC: mgreger@cinci.rr.com
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/specs/qcow2.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
index 80cdfd0..7049415 100644
--- a/docs/specs/qcow2.txt
+++ b/docs/specs/qcow2.txt
@@ -323,11 +323,16 @@ Standard Cluster Descriptor:

 Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):

-    Bit  0 -  x:    Host cluster offset. This is usually _not_ aligned to a
-                    cluster boundary!
+    Bit  0 -  x:    Bits 9-(x+9) of host cluster offset. This is
+                    usually _not_ aligned to a cluster boundary!

        x+1 - 61:    Compressed size of the images in sectors of 512 bytes

+The bits of the host cluster offset not specified in the cluster descriptor
+are 0 (bits 0-8 are obvious because a 512-byte sector is the smallest
+addressable unit, while bits 56-63 implies that a qcow2 file cannot exceed
+2^56 bytes in size).
+
 If a cluster is unallocated, read requests shall read the data from the backing
 file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
 no backing file or the backing file is smaller than the image, they shall read
-- 
2.5.0

Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: Clarify that compressed cluster offset requires shift

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1744 bytes --]

On 29.02.2016 16:11, Eric Blake wrote:
> The specs for the host cluster offset of a compressed cluster
> were not clear that the offset is in terms of sectors, and requires
> a shift by 9 to be a byte offset.  Add some more text to make the
> interpretation obvious.
> 
> CC: mgreger@cinci.rr.com
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  docs/specs/qcow2.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
> index 80cdfd0..7049415 100644
> --- a/docs/specs/qcow2.txt
> +++ b/docs/specs/qcow2.txt
> @@ -323,11 +323,16 @@ Standard Cluster Descriptor:
> 
>  Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
> 
> -    Bit  0 -  x:    Host cluster offset. This is usually _not_ aligned to a
> -                    cluster boundary!
> +    Bit  0 -  x:    Bits 9-(x+9) of host cluster offset. This is

I'd rather use one of "9..(x+9)", "[9, x+9]", "9 - (x+9)", "9—(x+9)" in
order to clarify that this is not a minus but a range.

Max

> +                    usually _not_ aligned to a cluster boundary!
> 
>         x+1 - 61:    Compressed size of the images in sectors of 512 bytes
> 
> +The bits of the host cluster offset not specified in the cluster descriptor
> +are 0 (bits 0-8 are obvious because a 512-byte sector is the smallest
> +addressable unit, while bits 56-63 implies that a qcow2 file cannot exceed
> +2^56 bytes in size).
> +
>  If a cluster is unallocated, read requests shall read the data from the backing
>  file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
>  no backing file or the backing file is smaller than the image, they shall read
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: Clarify that compressed cluster offset requires shift

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1311 bytes --]

On 29.02.2016 16:42, Max Reitz wrote:
> On 29.02.2016 16:11, Eric Blake wrote:
>> The specs for the host cluster offset of a compressed cluster
>> were not clear that the offset is in terms of sectors, and requires
>> a shift by 9 to be a byte offset.  Add some more text to make the
>> interpretation obvious.
>>
>> CC: mgreger@cinci.rr.com
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  docs/specs/qcow2.txt | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/specs/qcow2.txt b/docs/specs/qcow2.txt
>> index 80cdfd0..7049415 100644
>> --- a/docs/specs/qcow2.txt
>> +++ b/docs/specs/qcow2.txt
>> @@ -323,11 +323,16 @@ Standard Cluster Descriptor:
>>
>>  Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
>>
>> -    Bit  0 -  x:    Host cluster offset. This is usually _not_ aligned to a
>> -                    cluster boundary!
>> +    Bit  0 -  x:    Bits 9-(x+9) of host cluster offset. This is
> 
> I'd rather use one of "9..(x+9)", "[9, x+9]", "9 - (x+9)", "9—(x+9)" in
> order to clarify that this is not a minus but a range.

...unless, of course, this is actually a byte offset, which it seems to
be, judging from qcow2_get_cluster_offset() (and
qcow2_decompress_cluster(), as Kevin said on IRC).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: Clarify that compressed cluster offset requires shift

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

On 02/29/2016 08:46 AM, Max Reitz wrote:

>>>  Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
>>>
>>> -    Bit  0 -  x:    Host cluster offset. This is usually _not_ aligned to a
>>> -                    cluster boundary!
>>> +    Bit  0 -  x:    Bits 9-(x+9) of host cluster offset. This is
>>
>> I'd rather use one of "9..(x+9)", "[9, x+9]", "9 - (x+9)", "9—(x+9)" in
>> order to clarify that this is not a minus but a range.
> 
> ...unless, of course, this is actually a byte offset, which it seems to
> be, judging from qcow2_get_cluster_offset() (and
> qcow2_decompress_cluster(), as Kevin said on IRC).

Hmm, then I may be totally wrong. (Serves me right for trying to write a
spec patch without testing actual behavior).

At the least, we may want to say that things are usually not aligned to
even a _sector_ boundary (not just cluster boundary), and also clarify
whether the sector length is truncated or rounded up (zlib doesn't care
about trailing garbage during decompression, but you DO need to know how
many sectors to read to ensure that you are supplying zlib with trailing
garbage, rather than a truncated stream).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: Clarify that compressed cluster offset requires shift

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1274 bytes --]

On 29.02.2016 17:06, Eric Blake wrote:
> On 02/29/2016 08:46 AM, Max Reitz wrote:
> 
>>>>  Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
>>>>
>>>> -    Bit  0 -  x:    Host cluster offset. This is usually _not_ aligned to a
>>>> -                    cluster boundary!
>>>> +    Bit  0 -  x:    Bits 9-(x+9) of host cluster offset. This is
>>>
>>> I'd rather use one of "9..(x+9)", "[9, x+9]", "9 - (x+9)", "9—(x+9)" in
>>> order to clarify that this is not a minus but a range.
>>
>> ...unless, of course, this is actually a byte offset, which it seems to
>> be, judging from qcow2_get_cluster_offset() (and
>> qcow2_decompress_cluster(), as Kevin said on IRC).
> 
> Hmm, then I may be totally wrong. (Serves me right for trying to write a
> spec patch without testing actual behavior).
> 
> At the least, we may want to say that things are usually not aligned to
> even a _sector_ boundary (not just cluster boundary), and also clarify
> whether the sector length is truncated or rounded up (zlib doesn't care
> about trailing garbage during decompression, but you DO need to know how
> many sectors to read to ensure that you are supplying zlib with trailing
> garbage, rather than a truncated stream).

Yes, I agree.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]