Re: [Qemu-devel] [PATCH v2 3/5] block: initial VHDX driver support framework - supports open and probe

[Kevin Wolf <kwolf@redhat.com>]

Am 23.04.2013 um 18:11 hat Jeff Cody geschrieben:
> On Tue, Apr 23, 2013 at 05:46:28PM +0200, Kevin Wolf wrote:
> > Am 23.04.2013 um 16:24 hat Jeff Cody geschrieben:
> > > +/*
> > > + * Per the MS VHDX Specification, for every VHDX file:
> > > + *      - The header section is fixed size - 1 MB
> > > + *      - The header section is always the first "object"
> > > + *      - The first 64KB of the header is the File Identifier
> > > + *      - The first uint64 (8 bytes) is the VHDX Signature ("vhdxfile")
> > > + *      - The following 512 bytes constitute a UTF-16 string identifiying the
> > > + *        software that created the file, and is optional and diagnostic only.
> > > + *
> > > + *  Therefore, we probe by looking for the vhdxfile signature "vhdxfile"
> > > + */
> > > +static int vhdx_probe(const uint8_t *buf, int buf_size, const char *filename)
> > > +{
> > > +    if (buf_size >= 8 && !memcmp(buf, "vhdxfile", 8)) {
> > 
> > There's a VHDX_FILE_ID_MAGIC constant in the header. Don't you want to
> > use it?
> > 
> 
> Maybe I should remove all the magics from the header.  I could use it,
> but I think this is more readable and perhaps easier to follow against
> the spec.  For the upcoming log patches, there are other magic items
> that are compared against strings rather than the magic defines as
> well.
> 
> If you prefer I compared against the _MAGIC defines in the header, I
> have no problem with that.

I don't mind using the strings, but then I think removing the numeric
magic from the header is the right thing indeed.

> > > +    for (i = 0; i < s->bat_entries; i++) {
> > > +        le64_to_cpus(&s->bat[i]);
> > > +    }
> > > +
> > > +    if (flags & BDRV_O_RDWR) {
> > > +        ret = -ENOTSUP;
> > > +        goto fail;
> > > +    }
> > > +
> > > +    /* TODO: differencing files, read, write */
> > > +
> > > +    return 0;
> > > +fail:
> > > +    qemu_vfree(s->bat);
> > 
> > This doesn't consider all the structs that were allocated by functions
> > called in vhdx_open(). Here the same things should be freed as in
> > vhdx_close().
> > 
> 
> OK.  Those functions clean up after themselves, but you are right, if
> a failure happens later they should be cleaned up here as well.

Yes, you can probably get away with not freeing whatever the last
function call could have allocated if it cleans up after itself, but
doing the full thing here is more obviously correct. (You may have to
move the cleanup here instead of duplicating it, or make the functions'
cleanup reset the pointers to NULL)

Kevin