[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
Re: [Qemu-block] [Qemu-devel] [PATCH 03/20] qcow2: Extend spec for external data files
Wed, 6 Mar 2019 16:06:33 +0100
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.
Description: PGP signature