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

[Kevin Wolf <kwolf@redhat.com>]

Am 25.04.2013 um 17:03 hat Jeff Cody geschrieben:
> On Thu, Apr 25, 2013 at 03:04:23PM +0200, Kevin Wolf wrote:
> > Am 23.04.2013 um 16:24 hat Jeff Cody geschrieben:
> > > +    if (!vhdx_checksum_is_valid(buffer, VHDX_HEADER_BLOCK_SIZE, 4) ||
> > > +        s->rt.signature != VHDX_RT_MAGIC) {
> > > +        ret = -EINVAL;
> > > +        goto fail;
> > > +    }
> > > +
> > > +    for (i = 0; i < s->rt.entry_count; i++) {
> > > +        memcpy(&rt_entry, buffer+offset, sizeof(rt_entry));
> > > +        offset += sizeof(rt_entry);
> > > +
> > > +        leguid_to_cpus(&rt_entry.guid);
> > > +        le64_to_cpus(&rt_entry.file_offset);
> > > +        le32_to_cpus(&rt_entry.length);
> > > +        le32_to_cpus(&rt_entry.data_bits);
> > > +
> > > +        /* see if we recognize the entry */
> > > +        if (guid_eq(rt_entry.guid, bat_guid)) {
> > > +            s->bat_rt = rt_entry;
> > > +            continue;
> > > +        }
> > > +
> > > +        if (guid_eq(rt_entry.guid, metadata_guid)) {
> > > +            s->metadata_rt = rt_entry;
> > > +            continue;
> > > +        }
> > 
> > If the same regions occurs multiple times, the latest wins. Such images
> > aren't valid, but what should we do with such cases? Is this good enough
> > or should we detect it?
> 
> I think such images are more undefined rather than explicitly invalid.
> I don't think the spec touches on the idea of multiple regions of the
> same type.

It says "The Guid field specifies a 128-bit identifier for the object
and must be unique within the table.", so it's explicitly forbidden to
have two regions of the same type.

> That said, I don't what to make of an image file with
> multiple BAT regions, for instance - I think error is the only sane
> option.
> 
> Maybe keep a list of all entries, and if there are duplicates error
> out with -EINVAL?

We could do that if we really care. Or just check for the ones that we
recognise that they aren't set yet when we assign them.

> > > +    /* We now have the file parameters, so we can tell if this is a
> > > +     * differencing file (i.e.. has_parent), is dynamic or fixed
> > > +     * sized (leave_blocks_allocated), and the block size */
> > > +
> > > +    /* The parent locator required iff the file parameters has_parent set */
> > > +    if (s->params.data_bits & VHDX_PARAMS_HAS_PARENT) {
> > > +        if (s->metadata_entries.present & ~META_PARENT_LOCATOR_PRESENT) {
> > 
> > Not sure what you're trying to achieve here. We get -ENOTSUP if any
> > metadata entry except the parent locator is present, and -EINVAL if
> > there is none?
> >
> 
> That is a mistake - that check should be sans tilde:
>   if (s->metadata_entries.present & META_PARENT_LOCATOR_PRESENT) 

Right, that makes more sense.

Kevin