[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH 03/20] qcow2: Extend spec for exter

From: Kevin Wolf
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH 03/20] qcow2: Extend spec for external data files
Date: Wed, 6 Mar 2019 16:06:33 +0100
User-agent: Mutt/1.11.3 (2019-02-01)

Am 06.03.2019 um 13:43 hat Eric Blake geschrieben:
> On 3/6/19 3:51 AM, Stefan Hajnoczi wrote:
> > On Fri, Mar 01, 2019 at 10:32:27AM -0600, Eric Blake wrote:
> >> On 3/1/19 10:17 AM, Stefan Hajnoczi wrote:
> >>> On Wed, Feb 27, 2019 at 06:22:39PM +0100, Kevin Wolf wrote:
> >>>> @@ -148,6 +170,7 @@ be stored. Each extension has a structure like the 
> >>>> following:
> >>>>                          0x6803f857 - Feature name table
> >>>>                          0x23852875 - Bitmaps extension
> >>>>                          0x0537be77 - Full disk encryption header pointer
> >>>> +                        0x44415441 - External data file name
> >>>
> >>> This new header extension isn't described in this patch?
> >>
> >> I asked the same on v1, and the answer (which remains valid) is that
> >> neither is 0xe2792aca Backing file format name.  (In other words, both
> >> extensions are simple enough as a single file name to be implicitly
> >> described by the reference to the header in the earlier text).  Making
> >> both explicit wouldn't hurt my feelings, but I don't see it as a
> >> showstopper to the patch as-is.
> > 
> > The spec should make the representation clear.  Is it a NUL-terminated
> > string or is the length dictated by the header extension length field?
> My understanding is length determined by the header field, with optional
> NUL padding out to the alignment boundary (but that also means that it
> does NOT necessarily have a trailing NUL on disk if sizing matches
> alignment).  But yes, being explicit never hurts.
> > 
> > Otherwise implementors are forced to look at the QEMU source code or
> > guess based on hex dumping example files :(.
> Indeed, cleaning up the existing Backing file format name is worth doing
> (at which point this should follow suit). But it still sounds like a
> separate patch, at which point it becomes a question of ordering - if
> the cleanup lands first, then this needs to rebase to do the same; if
> this lands first, then the cleanup does both headers at once.

Maybe add a new section "String header extensions" that covers both?

If this remains the only patch in the series that would need a
significant change, I'd prefer a follow-up patch indeed.


Attachment: signature.asc
Description: PGP signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]