From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55628) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVPP0-0003nr-3b for qemu-devel@nongnu.org; Thu, 25 Apr 2013 12:52:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVPOv-0004kh-EK for qemu-devel@nongnu.org; Thu, 25 Apr 2013 12:52:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53751) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVPOv-0004kY-7a for qemu-devel@nongnu.org; Thu, 25 Apr 2013 12:52:05 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3PGq4HG008126 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 25 Apr 2013 12:52:04 -0400 Date: Thu, 25 Apr 2013 18:52:02 +0200 From: Kevin Wolf Message-ID: <20130425165202.GB2486@dhcp-200-207.str.redhat.com> References: <56a0e0347fde5396fb6b2a260de5b1a867a588d6.1366726446.git.jcody@redhat.com> <20130425130423.GB3156@dhcp-200-207.str.redhat.com> <20130425150338.GB3525@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130425150338.GB3525@localhost.localdomain> Subject: Re: [Qemu-devel] [PATCH v2 3/5] block: initial VHDX driver support framework - supports open and probe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: qemu-devel@nongnu.org, stefanha@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