Re: [PATCH] qcow2: Explicit mention of padding bytes

[Eric Blake]

On Mon, May 22, 2023 at 11:26:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 22.05.23 21:46, Eric Blake wrote:
> > Although we already covered the need for padding bytes with our
> > changes in commit 3ae3fcfa, commit 66fcbca5 (both v5.0.0) added one
> > byte and relied on the rest of the text for implicitly covering 7
> > padding bytes.  For consistency with other parts of the header (such
> > as the header extension format listing padding from n - m, or the
> > snapshot table entry listing variable padding), we might as well call
> > out the remaining 7 bytes as padding until such time (as any) as they
> > gain another meaning.
> > 
> > Signed-off-by: Eric Blake 
> > CC: Vladimir Sementsov-Ogievskiy 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Not strictly related to NBD, but I'll pick it up since I'm about to do a pull 
request.

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

Re: [PATCH] qcow2: Explicit mention of padding bytes

[Vladimir Sementsov-Ogievskiy]

On 22.05.23 21:46, Eric Blake wrote:

Although we already covered the need for padding bytes with our
changes in commit 3ae3fcfa, commit 66fcbca5 (both v5.0.0) added one
byte and relied on the rest of the text for implicitly covering 7
padding bytes.  For consistency with other parts of the header (such
as the header extension format listing padding from n - m, or the
snapshot table entry listing variable padding), we might as well call
out the remaining 7 bytes as padding until such time (as any) as they
gain another meaning.

Signed-off-by: Eric Blake 
CC: Vladimir Sementsov-Ogievskiy 


Reviewed-by: Vladimir Sementsov-Ogievskiy 


---

Spring cleaning my old branches.

v3: reviving an old patch; v2 was:
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg00687.html
---
  docs/interop/qcow2.txt | 1 +
  1 file changed, 1 insertion(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index e7f036c286b..2c4618375ad 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -226,6 +226,7 @@ version 2.
   in QEMU. However, clusters with 
the
  deflate compression type do not have zlib headers.

+105 - 111:  Padding, contents defined below.




--
Best regards,
Vladimir

Re: [PATCH] qcow2: Explicit mention of padding bytes

[Vladimir Sementsov-Ogievskiy]

27.02.2020 17:45, Eric Blake wrote:

Although we already covered the need for padding bytes with our
changes in commit 3ae3fcfa, commit 66fcbca5 just added one byte and
relied on the earlier text for implicitly covering 7 padding bytes.
For consistency with other parts of the header, it does not hurt to be
explicit that this version of the header is using padding bytes, and
if we later add other extension fields, we can rework the allocation
of those padding bytes to match those additions.

Signed-off-by: Eric Blake 
---
  docs/interop/qcow2.txt | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e244746e..193ac7c5c1af 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -209,6 +209,8 @@ version 2.
  Available compression type values:
  0: zlib 

+  105 - m:  Zero padding to round up the header size to the next
+multiple of 8.



Hmm. Strictly speaking, you defined it as one of additional fields. And by
this definition, we may start to check in qemu that these bytes are zero,
instead of ignoring them and keeping as is..

But may be it's just a nitpicking..


--
Best regards,
Vladimir

Re: [PATCH] qcow2: Explicit mention of padding bytes

[Max Reitz]

On 27.02.20 15:45, Eric Blake wrote:
> Although we already covered the need for padding bytes with our
> changes in commit 3ae3fcfa, commit 66fcbca5 just added one byte and
> relied on the earlier text for implicitly covering 7 padding bytes.
> For consistency with other parts of the header,

Those other places are about table entries where there is padding to the
next entry of the table, though.  In addition, for those other places
it’s the only mention that they are padded.

For the header, there is a whole own section describing the padding, so
I don’t quite see the point.

> it does not hurt to be
> explicit that this version of the header is using padding bytes, and
> if we later add other extension fields, we can rework the allocation
> of those padding bytes to match those additions.
> 
> Signed-off-by: Eric Blake 
> ---
>  docs/interop/qcow2.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 5597e244746e..193ac7c5c1af 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -209,6 +209,8 @@ version 2.
>  Available compression type values:
>  0: zlib 
> 
> +  105 - m:  Zero padding to round up the header size to the next
> +multiple of 8.

And if we really want this, I think it might as well be specific, i.e.
“105 - 112”.  We would have to adjust it every time we add a new field
anyway.

Max

>  === Header padding ===
> 




signature.asc
Description: OpenPGP digital signature