From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VCAU8-00080j-DP for qemu-devel@nongnu.org; Wed, 21 Aug 2013 11:38:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VCAU2-0001lF-Df for qemu-devel@nongnu.org; Wed, 21 Aug 2013 11:38:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VCAU2-0001ku-5u for qemu-devel@nongnu.org; Wed, 21 Aug 2013 11:38:06 -0400 Date: Wed, 21 Aug 2013 11:38:01 -0400 From: Jeff Cody Message-ID: <20130821153801.GA12194@localhost.localdomain> References: <3480ba616f83aa60eeb31258cb4b56cd14f59c60.1376976937.git.jcody@redhat.com> <20130821150930.GA18303@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130821150930.GA18303@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 07/13] block: vhdx - log parsing, replay, and flush support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Wed, Aug 21, 2013 at 05:09:30PM +0200, Stefan Hajnoczi wrote: > On Tue, Aug 20, 2013 at 02:01:18AM -0400, Jeff Cody wrote: > > Will require more iterations of review, but here's what I have so far: > Yeah, I assumed it would take a few iterations of review. > > +/* Returns true if the GUID is zero */ > > +static bool vhdx_log_guid_is_zero(MSGUID *guid) > > +{ > > + int i; > > + int ret = 0; > > + > > + /* If either the log guid, or log length is zero, > > + * then a replay log is not present */ > > The comment about log length here is not relevant to this function. > > > + for (i = 0; i < sizeof(MSGUID); i++) { > > + ret |= ((uint8_t *) guid)[i]; > > + } > > + > > + return ret == 0; > > +} > > IMO there is no need for this function. Just declare a const MSGUID > zero_guid = {0} global and use memcmp(): > > is_zero = guid_eq(guid, zero_guid); > OK, sounds good to me. > > +static bool vhdx_log_desc_is_valid(VHDXLogDescriptor *desc, > > + VHDXLogEntryHeader *hdr) > > +{ > > + bool ret = false; > > + > > + if (desc->sequence_number != hdr->sequence_number) { > > + goto exit; > > + } > > + if (desc->file_offset % VHDX_LOG_SECTOR_SIZE) { > > + goto exit; > > + } > > + > > + if (!memcmp(&desc->signature, "zero", 4)) { > > + if (!desc->zero_length % VHDX_LOG_SECTOR_SIZE) { > > Precedence looks wrong here, did you mean: > > if (desc->zero_length % VHDX_LOG_SECTOR_SIZE == 0) { > You are correct. The precedence is wrong - the modulo should have been encapsulated in (), but the explicit == is better. This never triggered anything in my testing because the internal code does not use zero descriptors, and none of the logs I was able to produce from Hyper-V contained zero descriptors. > > +static int vhdx_log_search(BlockDriverState *bs, BDRVVHDXState *s, > > + VHDXLogSequence *logs) > > +{ > > + int ret = 0; > > + > > + uint64_t curr_seq = 0; > > + VHDXLogSequence candidate = { 0 }; > > + VHDXLogSequence current = { 0 }; > > + > > + uint32_t tail; > > + bool seq_valid = false; > > + VHDXLogEntryHeader hdr = { 0 }; > > + VHDXLogEntries curr_log; > > + > > + memcpy(&curr_log, &s->log, sizeof(VHDXLogEntries)); > > + curr_log.write = curr_log.length; /* assume log is full */ > > + curr_log.read = 0; > > + > > + > > + /* now we will go through the whole log sector by sector, until > > + * we find a valid, active log sequence, or reach the end of the > > + * log buffer */ > > + for (;;) { > > + tail = curr_log.read; > > + > > + curr_seq = 0; > > + memset(¤t, 0, sizeof(current)); > > You could declare curr_seq, current, and friends inside the for loop > scope to avoid duplicate initializations. > OK > > +int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s) > > +{ > > + int ret = 0; > > + VHDXHeader *hdr; > > + VHDXLogSequence logs = { 0 }; > > + > > + hdr = s->headers[s->curr_header]; > > + > > + /* s->log.hdr is freed in vhdx_close() */ > > vhdx_close() is not called when .bdrv_open() fails so s->log.hdr is > leaked. > Good point. I'll look and see to make sure, but I think I can just have vhdx_open() call vhdx_close() to cleanup on error, assuming everything is NULL initialized. > > + if (s->log.hdr == NULL) { > > + s->log.hdr = qemu_blockalign(bs, sizeof(VHDXLogEntryHeader)); > > + } > > + > > + s->log.offset = hdr->log_offset; > > + s->log.length = hdr->log_length; > > + > > + if (s->log.offset < VHDX_LOG_MIN_SIZE || > > + s->log.offset % VHDX_LOG_MIN_SIZE) { > > + ret = -EINVAL; > > + goto exit; > > + } > > To be completely safe we should probably ensure that the log does not > overlap any other structures, as mentioned in the spec. I agree. Thanks for the review, Jeff