From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54847) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUfvE-0007qj-1a for qemu-devel@nongnu.org; Tue, 23 Apr 2013 12:18:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UUfvC-000608-N1 for qemu-devel@nongnu.org; Tue, 23 Apr 2013 12:18:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61976) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUfvC-0005zv-D2 for qemu-devel@nongnu.org; Tue, 23 Apr 2013 12:18:22 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3NGILWt009900 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 23 Apr 2013 12:18:21 -0400 Date: Tue, 23 Apr 2013 18:18:20 +0200 From: Kevin Wolf Message-ID: <20130423161820.GA28008@dhcp-200-207.str.redhat.com> References: <56a0e0347fde5396fb6b2a260de5b1a867a588d6.1366726446.git.jcody@redhat.com> <20130423154628.GC9989@dhcp-200-207.str.redhat.com> <20130423161120.GA4131@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130423161120.GA4131@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 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