From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4xbC-00017U-M4 for qemu-devel@nongnu.org; Thu, 01 Aug 2013 14:27:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4tXF-0005zu-8e for qemu-devel@nongnu.org; Thu, 01 Aug 2013 10:07:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20857) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4tBM-0005jp-4P for qemu-devel@nongnu.org; Thu, 01 Aug 2013 09:44:44 -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 r71DihJk023725 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 1 Aug 2013 09:44:43 -0400 Date: Thu, 1 Aug 2013 15:44:41 +0200 From: Stefan Hajnoczi Message-ID: <20130801134441.GD2387@stefanha-thinkpad.redhat.com> References: <595d71cf1cd25ac9deff521bceb82b8c3da888d0.1375325971.git.jcody@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <595d71cf1cd25ac9deff521bceb82b8c3da888d0.1375325971.git.jcody@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 2/9] block: vhdx - add header update capability. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org On Wed, Jul 31, 2013 at 11:23:47PM -0400, Jeff Cody wrote: > @@ -212,6 +242,24 @@ bool vhdx_checksum_is_valid(uint8_t *buf, size_t size, int crc_offset) > > > /* > + * This generates a UUID that is compliant with the MS GUIDs used > + * in the VHDX spec (and elsewhere). > + * > + * We can do this with uuid_generate if uuid.h is present, > + * however not all systems have uuid and the generation is > + * pretty straightforward for the DCE + random usage case The talk of uuid.h not being available confuses me. The code has no #ifdef and we do not define uuid_generate() ourselves. Is this an outdated comment? > +/* Update the VHDX headers > + * > + * This follows the VHDX spec procedures for header updates. > + * > + * - non-current header is updated with largest sequence number > + */ > +static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s, bool rw) "rw" should be named "generate_file_write_guid". If you call vhdx_update_header() again later you do not need to update FileWriteGuid again according to the spec. There's probably no harm in doing so but the spec explicitly says "If this is the first header update within the session, use a new value for the FileWriteGuid". This means that future calls to vhdx_update_header() in the same session should set "generate_file_write_guid" to false. Therefore "rw" is not the right name. > +{ > + int ret = 0; > + int hdr_idx = 0; > + uint64_t header_offset = VHDX_HEADER1_OFFSET; > + > + VHDXHeader *active_header; > + VHDXHeader *inactive_header; > + VHDXHeader header_le; > + uint8_t *buffer; > + > + /* operate on the non-current header */ > + if (s->curr_header == 0) { > + hdr_idx = 1; > + header_offset = VHDX_HEADER2_OFFSET; > + } > + > + active_header = s->headers[s->curr_header]; > + inactive_header = s->headers[hdr_idx]; > + > + inactive_header->sequence_number = active_header->sequence_number + 1; > + > + /* a new file guid must be generate before any file write, including s/generate/generated/ > + * headers */ > + memcpy(&inactive_header->file_write_guid, &s->session_guid, > + sizeof(MSGUID)); > + > + /* a new data guid only needs to be generate before any guest-visible > + * writes, so update it if the image is opened r/w. */ > + if (rw) { > + vhdx_guid_generate(&inactive_header->data_write_guid); > + } > + > + /* the header checksum is not over just the packed size of VHDXHeader, > + * but rather over the entire 'reserved' range for the header, which is > + * 4KB (VHDX_HEADER_SIZE). */ > + > + buffer = qemu_blockalign(bs, VHDX_HEADER_SIZE); > + /* we can't assume the extra reserved bytes are 0 */ > + ret = bdrv_pread(bs->file, header_offset, buffer, VHDX_HEADER_SIZE); > + if (ret < 0) { > + goto exit; > + } > + /* overwrite the actual VHDXHeader portion */ > + memcpy(buffer, inactive_header, sizeof(VHDXHeader)); > + inactive_header->checksum = vhdx_update_checksum(buffer, > + VHDX_HEADER_SIZE, 4); > + vhdx_header_le_export(inactive_header, &header_le); > + bdrv_pwrite_sync(bs->file, header_offset, &header_le, sizeof(VHDXHeader)); This function can fail and it's a good indicator that future I/Os will also be in trouble. Please propagate the error return. > @@ -801,8 +955,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags) > } > > if (flags & BDRV_O_RDWR) { > - ret = -ENOTSUP; > - goto fail; > + vhdx_update_headers(bs, s, false); Error handling is missing. At this point it looks like we cannot write to the file. Propagating the error seems reasonable.