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

[Kevin Wolf]

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.

Kevin

signature.asc
Description: PGP signature