From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54075) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQyBB-0008GS-Aj for qemu-devel@nongnu.org; Tue, 01 Oct 2013 07:31:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VQyB2-0000Qf-SZ for qemu-devel@nongnu.org; Tue, 01 Oct 2013 07:31:49 -0400 Received: from mail-wg0-x233.google.com ([2a00:1450:400c:c00::233]:47116) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQyB2-0000QX-Cq for qemu-devel@nongnu.org; Tue, 01 Oct 2013 07:31:40 -0400 Received: by mail-wg0-f51.google.com with SMTP id c11so6978960wgh.6 for ; Tue, 01 Oct 2013 04:31:39 -0700 (PDT) Date: Tue, 1 Oct 2013 13:31:36 +0200 From: Stefan Hajnoczi Message-ID: <20131001113136.GA6035@stefanha-thinkpad.redhat.com> References: <88c4403314c6c58ac4fe672e252c5c4a3e4c61ca.1380141614.git.jcody@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <88c4403314c6c58ac4fe672e252c5c4a3e4c61ca.1380141614.git.jcody@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 08/20] block: vhdx - log parsing, replay, and flush support 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, stefanha@redhat.com On Wed, Sep 25, 2013 at 05:02:53PM -0400, Jeff Cody wrote: > +static int vhdx_log_read_desc(BlockDriverState *bs, BDRVVHDXState *s, > + VHDXLogEntries *log, VHDXLogDescEntries **buffer) > +{ > + int ret = 0; > + uint32_t desc_sectors; > + uint32_t sectors_read; > + VHDXLogEntryHeader hdr; > + VHDXLogDescEntries *desc_entries = NULL; > + int i; > + > + assert(*buffer == NULL); > + > + ret = vhdx_log_peek_hdr(bs, log, &hdr); > + if (ret < 0) { > + goto exit; > + } > + vhdx_log_entry_hdr_le_import(&hdr); > + if (vhdx_log_hdr_is_valid(log, &hdr, s) == false) { > + ret = -EINVAL; > + goto exit; > + } > + > + desc_sectors = vhdx_compute_desc_sectors(hdr.descriptor_count); I don't see input validation for hdr.descriptor_count. It not exceed log length. > +static int vhdx_log_flush_desc(BlockDriverState *bs, VHDXLogDescriptor *desc, > + VHDXLogDataSector *data) > +{ > + int ret = 0; > + uint64_t seq, file_offset; > + uint32_t offset = 0; > + void *buffer = NULL; > + uint64_t count = 1; > + int i; > + > + buffer = qemu_blockalign(bs, VHDX_LOG_SECTOR_SIZE); > + > + if (!memcmp(&desc->signature, "desc", 4)) { > + /* data sector */ > + if (data == NULL) { > + ret = -EFAULT; > + goto exit; > + } > + > + /* The sequence number of the data sector must match that > + * in the descriptor */ > + seq = data->sequence_high; > + seq <<= 32; > + seq |= data->sequence_low & 0xffffffff; > + > + if (seq != desc->sequence_number) { > + ret = -EINVAL; > + goto exit; > + } > + > + /* Each data sector is in total 4096 bytes, however the first > + * 8 bytes, and last 4 bytes, are located in the descriptor */ > + memcpy(buffer, &desc->leading_bytes, 8); > + offset += 8; > + > + memcpy(buffer+offset, data->data, 4084); > + offset += 4084; > + > + memcpy(buffer+offset, &desc->trailing_bytes, 4); > + > + } else if (!memcmp(&desc->signature, "zero", 4)) { > + /* write 'count' sectors of sector */ > + memset(buffer, 0, VHDX_LOG_SECTOR_SIZE); > + count = desc->zero_length / VHDX_LOG_SECTOR_SIZE; Zero descriptors also have a sequence number that should be checked. > +/* Parse the replay log. Per the VHDX spec, if the log is present > + * it must be replayed prior to opening the file, even read-only. > + * > + * If read-only, we must replay the log in RAM (or refuse to open > + * a dirty VHDX file read-only */ read-only) <-- missing parenthesis > @@ -794,10 +753,12 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, > uint32_t i; > uint64_t signature; > uint32_t data_blocks_cnt, bitmap_blocks_cnt; > + bool log_flushed = false; > > > s->bat = NULL; > s->first_visible_write = true; > + s->log.write = s->log.read = 0; This is not really necessary since bs->opaque is zeroed before block.c calls vhdx_open(). > > qemu_co_mutex_init(&s->lock); > > @@ -821,20 +782,33 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, > goto fail; > } > > - ret = vhdx_parse_log(bs, s); > + ret = vhdx_open_region_tables(bs, s); > if (ret) { > goto fail; > } > > - ret = vhdx_open_region_tables(bs, s); > + ret = vhdx_parse_metadata(bs, s); > if (ret) { > goto fail; > } > > - ret = vhdx_parse_metadata(bs, s); > + ret = vhdx_parse_log(bs, s, &log_flushed); > if (ret) { > goto fail; > } Why replay the log *after* reading logged metadata? We could read stale or corrupted values. I guess there is a dependency here - the log replay code needs some field that vhdx_open_region_tables() or vhdx_parse_metadata() load?