From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:37429)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VCA2e-0000Cs-PU
	for qemu-devel@nongnu.org; Wed, 21 Aug 2013 11:09:57 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VCA2W-0006wx-C6
	for qemu-devel@nongnu.org; Wed, 21 Aug 2013 11:09:48 -0400
Received: from mail-wg0-x232.google.com ([2a00:1450:400c:c00::232]:41088)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1VCA2W-0006wn-3A
	for qemu-devel@nongnu.org; Wed, 21 Aug 2013 11:09:40 -0400
Received: by mail-wg0-f50.google.com with SMTP id m15so499197wgh.5
	for <qemu-devel@nongnu.org>; Wed, 21 Aug 2013 08:09:39 -0700 (PDT)
Date: Wed, 21 Aug 2013 17:09:30 +0200
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130821150930.GA18303@stefanha-thinkpad.redhat.com>
References: <cover.1376976937.git.jcody@redhat.com>
	<3480ba616f83aa60eeb31258cb4b56cd14f59c60.1376976937.git.jcody@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3480ba616f83aa60eeb31258cb4b56cd14f59c60.1376976937.git.jcody@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v4 07/13] block: vhdx - log parsing, replay,
 and flush support
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Jeff Cody <jcody@redhat.com>
Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com

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:

> +/* 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);

> +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) {

> +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(&current, 0, sizeof(current));

You could declare curr_seq, current, and friends inside the for loop
scope to avoid duplicate initializations.

> +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.

> +    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.