Re: [Qemu-devel] [PATCH 7/9] block: vhdx - log parsing, replay, and flush support

[Jeff Cody]

On Tue, Jul 30, 2013 at 11:48:15AM +0800, Fam Zheng wrote:
> On Wed, 07/24 13:54, Jeff Cody wrote:
> > This adds support for VHDX v0 logs, as specified in Microsoft's
> > VHDX Specification Format v1.00:
> > https://www.microsoft.com/en-us/download/details.aspx?id=34750
> > 
> > The following support is added:
> > 
> > * Log parsing, and validation - validate that an existing log
> >   is correct.
> > 
> > * Log search - search through an existing log, to find any valid
> >   sequence of entries.
> > 
> > * Log replay and flush - replay an existing log, and flush/clear
> >   the log when complete.
> > 
> > The VHDX log is a circular buffer, with elements (sectors) of 4KB.
> > 
> > A log entry is a variably-length number of sectors, that is
> > comprised of a header and 'descriptors', that describe each sector.
> > 
> > A log may contain multiple entries, know as a log sequence.  In a log
> > sequence, each log entry immediately follows the previous entry, with an
> > incrementing sequence number.  There can only ever be one active and
> > valid sequence in the log.
> > 
> > Each log entry must match the file log GUID in order to be valid (along
> > with other criteria).  Once we have flushed all valid log entries, we
> > marked the file log GUID to be zero, which indicates a buffer with no
> > valid entries.
> > 
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> >  block/Makefile.objs |   2 +-
> >  block/vhdx-log.c    | 734 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  block/vhdx.c        |  44 +---
> >  block/vhdx.h        |   7 +-
> >  4 files changed, 743 insertions(+), 44 deletions(-)
> >  create mode 100644 block/vhdx-log.c
> > 
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index e6f5d33..2fbd79a 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -2,7 +2,7 @@ block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o 
> > dmg.o bochs.o vpc.o vvfat
> >  block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o 
> > qcow2-cache.o
> >  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
> >  block-obj-y += qed-check.o
> > -block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o
> > +block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
> >  block-obj-y += parallels.o blkdebug.o blkverify.o
> >  block-obj-y += snapshot.o qapi.o
> >  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
> > diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> > new file mode 100644
> > index 0000000..89b9000
> > --- /dev/null
> > +++ b/block/vhdx-log.c
> > @@ -0,0 +1,734 @@
> > +/*
> > + * Block driver for Hyper-V VHDX Images
> > + *
> > + * Copyright (c) 2013 Red Hat, Inc.,
> > + *
> > + * Authors:
> > + *  Jeff Cody <address@hidden>
> > + *
> > + *  This is based on the "VHDX Format Specification v1.00", published 
> > 8/25/2012
> > + *  by Microsoft:
> > + *      https://www.microsoft.com/en-us/download/details.aspx?id=34750
> > + *
> > + * This file covers the functionality of the metadata log writing, 
> > parsing, and
> > + * replay.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2 or 
> > later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + *
> > + */
> > +#include "qemu-common.h"
> > +#include "block/block_int.h"
> > +#include "qemu/module.h"
> > +#include "block/vhdx.h"
> > +
> > +
> > +typedef struct VHDXLogSequence {
> > +    bool valid;
> > +    uint32_t count;
> > +    VHDXLogEntries log;
> > +    VHDXLogEntryHeader hdr;
> > +} VHDXLogSequence;
> > +
> > +typedef struct VHDXLogDescEntries {
> > +    VHDXLogEntryHeader hdr;
> > +    VHDXLogDescriptor desc[];
> > +} VHDXLogDescEntries;
> > +
> > +
> > +/* 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 */
> > +    for (i = 0; i < sizeof(MSGUID); i++) {
> > +        ret |= ((uint8_t *) guid)[i];
> > +    }
> > +
> > +    return ret == 0;
> > +}
> > +
> > +/* The log located on the disk is circular buffer containing
> > + * sectors of 4096 bytes each.
> > + *
> > + * It is assumed for the read/write functions below that the
> > + * circular buffer scheme uses a 'one sector open' to indicate
> > + * the buffer is full.  Given the validation methods used for each
> > + * sector, this method should be compatible with other methods that
> > + * do not waste a sector.
> > + */
> > +
> > +
> > +/* Allow peeking at the hdr entry at the beginning of the current
> > + * read index, without advancing the read index */
> > +static int vhdx_log_peek_hdr(BlockDriverState *bs, VHDXLogEntries *log,
> > +                             VHDXLogEntryHeader *hdr)
> > +{
> > +    int ret = 0;
> > +    uint64_t offset;
> > +    uint32_t read;
> > +
> > +    assert(hdr != NULL);
> > +
> > +    /* peek is only support on sector boundaries */
> 
> s/support/supported/
>

Thanks

> > +    if (log->read % VHDX_LOG_SECTOR_SIZE) {
> > +        ret = -EFAULT;
> > +        goto exit;
> > +    }
> > +
> > +    read = log->read;
> > +    /* we are guaranteed that a) log sectors are 4096 bytes,
> > +     * and b) the log length is a multiple of 1MB. So, there
> > +     * is always a round number of sectors in the buffer */
> > +    if ((read + sizeof(VHDXLogEntryHeader)) > log->length) {
> > +        read = 0;
> > +    }
> > +
> > +    if (read == log->write) {
> > +        ret = -EINVAL;
> > +        goto exit;
> > +    }
> > +
> > +    offset = log->offset + read;
> > +
> > +    ret = bdrv_pread(bs->file, offset, hdr, sizeof(VHDXLogEntryHeader));
> > +    if (ret < 0) {
> > +        goto exit;
> > +    }
> > +
> > +exit:
> > +    return ret;
> > +}
> > +
> > +/* Index increment for log, based on sector boundaries */
> > +static int vhdx_log_inc_idx(uint32_t idx, uint64_t length)
> > +{
> > +    idx += VHDX_LOG_SECTOR_SIZE;
> > +    /* we are guaranteed that a) log sectors are 4096 bytes,
> > +     * and b) the log length is a multiple of 1MB. So, there
> > +     * is always a round number of sectors in the buffer */
> > +    return idx >= length ? 0 : idx;
> > +}
> > +
> > +
> > +/* Reset the log to empty */
> > +static void vhdx_log_reset(BlockDriverState *bs, BDRVVHDXState *s)
> > +{
> > +    MSGUID guid = { 0 };
> > +    s->log.read = s->log.write = 0;
> > +    /* a log guid of 0 indicates an empty log to any parser of v0
> > +     * VHDX logs */
> > +    vhdx_update_headers(bs, s, false, &guid);
> > +}
> > +
> > +/* Reads num_sectors from the log (all log sectors are 4096 bytes),
> > + * into buffer 'buffer'.  Upon return, *sectors_read will contain
> > + * the number of sectors successfully read.
> > + *
> > + * It is assumed that 'buffer' is already allocated, and of sufficient
> > + * size (i.e. >= 4096*num_sectors).
> > + *
> > + * If 'peek' is true, then the tail (read) pointer for the circular buffer 
> > is
> > + * not modified.
> > + *
> > + * 0 is returned on success, -errno otherwise.  */
> > +static int vhdx_log_read_sectors(BlockDriverState *bs, VHDXLogEntries *log,
> > +                                 uint32_t *sectors_read, void *buffer,
> > +                                 uint32_t num_sectors, bool peek)
> > +{
> > +    int ret = 0;
> > +    uint64_t offset;
> > +    uint32_t read;
> > +
> > +    read = log->read;
> > +
> > +    *sectors_read = 0;
> > +    while (num_sectors) {
> > +        if (read == log->write) {
> > +            /* empty */
> > +            break;
> > +        }
> > +        offset = log->offset + read;
> > +
> > +        ret = bdrv_pread(bs->file, offset, buffer, VHDX_LOG_SECTOR_SIZE);
> > +        if (ret < 0) {
> > +            goto exit;
> > +        }
> > +        read = vhdx_log_inc_idx(read, log->length);
> > +
> > +        *sectors_read = *sectors_read + 1;
> > +        num_sectors--;
> > +    }
> > +
> > +exit:
> > +    if (!peek) {
> > +        log->read = read;
> > +    }
> > +    return ret;
> > +}
> > +
> > +/* Validates a log entry header */
> > +static bool vhdx_log_hdr_is_valid(VHDXLogEntries *log, VHDXLogEntryHeader 
> > *hdr,
> > +                                  BDRVVHDXState *s)
> > +{
> > +    int valid = false;
> > +
> > +    if (memcmp(&hdr->signature, "loge", 4)) {
> > +        goto exit;
> > +    }
> > +
> > +    /* if the individual entry length is larger than the whole log
> > +     * buffer, that is obviously invalid */
> > +    if (log->length < hdr->entry_length) {
> > +        goto exit;
> > +    }
> > +
> > +    /* length of entire entry must be in units of 4KB (log sector size) */
> > +    if (hdr->entry_length % (VHDX_LOG_SECTOR_SIZE)) {
> > +        goto exit;
> > +    }
> > +
> > +    /* per spec, sequence # must be > 0 */
> > +    if (hdr->sequence_number == 0) {
> > +        goto exit;
> > +    }
> > +
> > +    /* log entries are only valid if they match the file-wide log guid
> > +     * found in the active header */
> > +    if (!guid_eq(hdr->log_guid, s->headers[s->curr_header]->log_guid)) {
> > +        goto exit;
> > +    }
> > +
> > +    valid = true;
> > +
> > +exit:
> > +    return valid;
> > +}
> > +
> > +/*
> > + * Given a log header, this will validate that the descriptors and the
> > + * corresponding data sectors (if applicable)
> > + *
> > + * Validation consists of:
> > + *      1. Making sure the sequence numbers matches the entry header
> > + *      2. Verifying a valid signature ('zero' or desc' for descriptors)
> 
> s/ desc'/ 'desc'/
>

Thanks

> > + *      3. File offset field is a multiple of 4KB
> > + *      4. If a data descriptor, the corresponding data sector
> > + *         has its signature ('data') and matching sequence number
> > + *
> > + * 'desc' is the data buffer containing the descriptor
> > + * hdr is the log entry header
> 
> Please use gtkdoc format:
> 
> @desc: the data buffer ...
> @hdr:  the log entry header
>

Sure, I can do that.

> > + *
> > + * Returns true if valid
> > + */
> > +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) {
> > +            /* valid */
> > +            ret = true;
> > +        }
> > +    } else if (!memcmp(&desc->signature, "desc", 4)) {
> > +            /* valid */
> > +            ret = true;
> > +    }
> > +
> > +exit:
> > +    return ret;
> > +}
> > +
> > +
> > +/* Prior to sector data for a log entry, there is the header
> > + * and the descriptors referenced in the header:
> > + *
> > + * [] = 4KB sector
> > + *
> > + * [ hdr, desc ][   desc   ][ ... ][ data ][ ... ]
> > + *
> > + * The first sector in a log entry has a 64 byte header, and
> > + * up to 126 32-byte descriptors.  If more descriptors than
> > + * 126 are required, then subsequent sectors can have up to 128
> > + * descriptors.  Each sector is 4KB.  Data follows the descriptor
> > + * sectors.
> > + *
> > + * This will return the number of sectors needed to encompass
> > + * the passed number of descriptors in desc_cnt.
> > + *
> > + * This will never return 0, even if desc_cnt is 0.
> > + */
> > +static int vhdx_compute_desc_sectors(uint32_t desc_cnt)
> > +{
> > +    uint32_t desc_sectors;
> > +
> > +    desc_cnt += 2; /* account for header in first sector */
> > +    desc_sectors = desc_cnt / 128;
> > +    if (desc_cnt % 128) {
> > +        desc_sectors++;
> > +    }
> > +
> > +    return desc_sectors;
> > +}
> > +
> > +
> > +/* Reads the log header, and subsequent descriptors (if any).  This
> > + * will allocate all the space for buffer, which must be NULL when
> > + * passed into this function. Each descriptor will also be validated,
> > + * and error returned if any are invalid. */
> > +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);
> > +    desc_entries = qemu_blockalign(bs, desc_sectors * 
> > VHDX_LOG_SECTOR_SIZE);
> > +
> > +    ret = vhdx_log_read_sectors(bs, log, &sectors_read, desc_entries,
> > +                                desc_sectors, false);
> > +    if (ret < 0) {
> > +        goto free_and_exit;
> > +    }
> > +    if (sectors_read != desc_sectors) {
> > +        ret = -EINVAL;
> > +        goto free_and_exit;
> > +    }
> > +
> > +    /* put in proper endianness, and validate each desc */
> > +    for (i = 0; i < hdr.descriptor_count; i++) {
> > +        vhdx_log_desc_le_import(&desc_entries->desc[i]);
> > +        if (vhdx_log_desc_is_valid(&desc_entries->desc[i], &hdr) == false) 
> > {
> > +            ret = -EINVAL;
> > +            goto free_and_exit;
> > +        }
> > +    }
> > +
> > +    *buffer = desc_entries;
> > +    goto exit;
> > +
> > +free_and_exit:
> > +    qemu_vfree(desc_entries);
> > +exit:
> > +    return ret;
> > +}
> > +
> > +
> > +/* Flushes the descriptor described by desc to the VHDX image file.
> > + * If the descriptor is a data descriptor, than 'data' must be non-NULL,
> > + * and >= 4096 bytes (VHDX_LOG_SECTOR_SIZE), containing the data to be
> > + * written.
> > + *
> > + * Verification is performed to make sure the sequence numbers of a data
> > + * descriptor match the sequence number in the desc.
> > + *
> > + * For a zero descriptor, it may describe multiple sectors to fill with 
> > zeroes.
> > + * In this case, it should be noted that zeroes are written to disk, and 
> > the
> > + * image file is not extended as a sparse file.  */
> > +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, sizeof(desc->leading_bytes));
> > +        offset += sizeof(desc->leading_bytes);
> > +
> > +        memcpy(buffer+offset, data->data, 4084);
> > +        offset += 4084;
> 
> Could you use sizeof(data->data) instead of 4084?
>

I suppose I should be consistent, and either use the explicit sizes,
or use 'sizeof'.  I was on the fence on whether it is better for these
three variables to use sizeof() or explicit sizes.

> > +
> > +        memcpy(buffer+offset, &desc->trailing_bytes,
> > +               sizeof(desc->trailing_bytes));
> > +
> > +    } 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;
> > +    }
> > +
> > +    file_offset = desc->file_offset;
> > +
> > +    /* count is only > 1 if we are writing zeroes */
> > +    for (i = 0; i < count; i++) {
> > +        ret = bdrv_pwrite_sync(bs->file, file_offset, buffer,
> > +                               VHDX_LOG_SECTOR_SIZE);
> > +        if (ret < 0) {
> > +            goto exit;
> > +        }
> > +        file_offset += VHDX_LOG_SECTOR_SIZE;
> > +    }
> > +
> > +exit:
> > +    qemu_vfree(buffer);
> > +    return ret;
> > +}
> > +
> > +/* Flush the entire log (as described by 'logs') to the VHDX image
> > + * file, and then set the log to 'empty' status once complete.
> > + *
> > + * The log entries should be validate prior to flushing */
> > +static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
> > +                          VHDXLogSequence *logs)
> > +{
> > +    int ret = 0;
> > +    int i;
> > +    uint32_t cnt, sectors_read;
> > +    uint64_t new_file_size;
> > +    void *data = NULL;
> > +    VHDXLogDescEntries *desc_entries = NULL;
> > +    VHDXLogEntryHeader hdr_tmp = { 0 };
> > +
> > +    cnt = logs->count;
> > +
> > +    data = qemu_blockalign(bs, VHDX_LOG_SECTOR_SIZE);
> > +
> > +    vhdx_user_visible_write(bs, s);
> > +
> > +    /* each iteration represents one log sequence, which may span multiple
> > +     * sectors */
> > +    while (cnt--) {
> > +        ret = vhdx_log_peek_hdr(bs, &logs->log, &hdr_tmp);
> > +        if (ret < 0) {
> > +            goto exit;
> > +        }
> > +        /* if the log shows a FlushedFileOffset larger than our current 
> > file
> > +         * size, then that means the file has been truncated / corrupted, 
> > and
> > +         * we must refused to open it / use it */
> > +        if (hdr_tmp.flushed_file_offset > bdrv_getlength(bs->file)) {
> > +            ret = -EINVAL;
> > +            goto exit;
> > +        }
> > +
> > +        ret = vhdx_log_read_desc(bs, s, &logs->log, &desc_entries);
> > +        if (ret < 0) {
> > +            goto exit;
> > +        }
> > +
> > +        for (i = 0; i < desc_entries->hdr.descriptor_count; i++) {
> > +            if (!memcmp(&desc_entries->desc[i].signature, "desc", 4)) {
> > +                /* data sector, so read a sector to flush */
> > +                ret = vhdx_log_read_sectors(bs, &logs->log, &sectors_read,
> > +                                            data, 1, false);
> > +                if (ret < 0) {
> > +                    goto exit;
> > +                }
> > +                if (sectors_read != 1) {
> > +                    ret = -EINVAL;
> > +                    goto exit;
> > +                }
> > +            }
> > +
> > +            ret = vhdx_log_flush_desc(bs, &desc_entries->desc[i], data);
> > +            if (ret < 0) {
> > +                goto exit;
> > +            }
> > +        }
> > +        if (bdrv_getlength(bs->file) < desc_entries->hdr.last_file_offset) 
> > {
> > +            new_file_size = desc_entries->hdr.last_file_offset;
> > +            if (new_file_size % (1024*1024)) {
> > +                /* round up to nearest 1MB boundary */
> > +                new_file_size = ((new_file_size >> 20) + 1) << 20;
> > +                bdrv_truncate(bs->file, new_file_size);
> > +            }
> > +        }
> > +        qemu_vfree(desc_entries);
> > +        desc_entries = NULL;
> > +    }
> > +
> > +    /* once the log is fully flushed, indicate that we have an empty log
> > +     * now.  This also sets the log guid to 0, to indicate an empty log */
> > +    vhdx_log_reset(bs, s);
> > +
> > +exit:
> > +    qemu_vfree(data);
> > +    qemu_vfree(desc_entries);
> > +    return ret;
> > +}
> > +
> > +static int vhdx_validate_log_entry(BlockDriverState *bs, BDRVVHDXState *s,
> > +                                   VHDXLogEntries *log, uint64_t seq,
> > +                                   bool *valid, VHDXLogEntryHeader *entry)
> > +{
> > +    int ret = 0;
> > +    VHDXLogEntryHeader hdr;
> > +    void *buffer = NULL;
> > +    uint32_t i, desc_sectors, total_sectors, crc;
> > +    uint32_t sectors_read = 0;
> > +    VHDXLogDescEntries *desc_buffer = NULL;
> > +
> > +    *valid = false;
> > +
> > +    ret = vhdx_log_peek_hdr(bs, log, &hdr);
> > +    if (ret < 0) {
> > +        goto inc_and_exit;
> > +    }
> > +
> > +    vhdx_log_entry_hdr_le_import(&hdr);
> > +
> > +
> > +    if (vhdx_log_hdr_is_valid(log, &hdr, s) == false) {
> > +        goto inc_and_exit;
> > +    }
> > +
> > +    if (seq > 0) {
> > +        if (hdr.sequence_number != seq + 1) {
> > +            goto inc_and_exit;
> > +        }
> > +    }
> > +
> > +    desc_sectors = vhdx_compute_desc_sectors(hdr.descriptor_count);
> > +
> > +    /* Read desc sectors, and calculate log checksum */
> > +
> > +    total_sectors = hdr.entry_length / VHDX_LOG_SECTOR_SIZE;
> > +
> > +
> > +    /* read_desc() will incrememnt the read idx */
> > +    ret = vhdx_log_read_desc(bs, s, log, &desc_buffer);
> > +    if (ret < 0) {
> > +        goto free_and_exit;
> > +    }
> > +
> > +    crc = vhdx_checksum_calc(0xffffffff, (void *)desc_buffer,
> > +                            desc_sectors * VHDX_LOG_SECTOR_SIZE, 4);
> > +    crc ^= 0xffffffff;
> > +
> > +    buffer = qemu_blockalign(bs, VHDX_LOG_SECTOR_SIZE);
> > +    if (total_sectors > desc_sectors) {
> > +        for (i = 0; i < total_sectors - desc_sectors; i++) {
> > +            sectors_read = 0;
> > +            ret = vhdx_log_read_sectors(bs, log, &sectors_read, buffer,
> > +                                        1, false);
> > +            if (ret < 0 || sectors_read != 1) {
> > +                goto free_and_exit;
> > +            }
> > +            crc = vhdx_checksum_calc(crc, buffer, VHDX_LOG_SECTOR_SIZE, 
> > -1);
> > +            crc ^= 0xffffffff;
> > +        }
> > +    }
> > +    crc ^= 0xffffffff;
> > +    if (crc != desc_buffer->hdr.checksum) {
> > +        goto free_and_exit;
> > +    }
> > +
> > +    *valid = true;
> > +    *entry = hdr;
> > +    goto free_and_exit;
> > +
> > +inc_and_exit:
> > +    log->read = vhdx_log_inc_idx(log->read, log->length);
> > +
> > +free_and_exit:
> > +    qemu_vfree(buffer);
> > +    qemu_vfree(desc_buffer);
> > +    return ret;
> > +}
> > +
> > +/* Search through the log circular buffer, and find the valid, active
> > + * log sequence, if any exists
> > + * */
> > +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));
> > +
> > +        ret = vhdx_validate_log_entry(bs, s, &curr_log, curr_seq,
> > +                                      &seq_valid, &hdr);
> > +        if (ret < 0) {
> > +            goto exit;
> > +        }
> > +
> > +        if (seq_valid) {
> > +            current.valid     = true;
> > +            current.log       = curr_log;
> > +            current.log.read  = tail;
> > +            current.log.write = curr_log.read;
> > +            current.count     = 1;
> > +            current.hdr       = hdr;
> > +
> > +
> > +            for (;;) {
> > +                ret = vhdx_validate_log_entry(bs, s, &curr_log, curr_seq,
> > +                                              &seq_valid, &hdr);
> > +                if (ret < 0) {
> > +                    goto exit;
> > +                }
> > +                if (seq_valid == false) {
> > +                    break;
> > +                }
> > +                current.log.write = curr_log.read;
> > +                current.count++;
> > +
> > +                curr_seq = hdr.sequence_number;
> > +            }
> > +        }
> > +
> > +        if (current.valid) {
> > +            if (candidate.valid == false ||
> > +                current.hdr.sequence_number > 
> > candidate.hdr.sequence_number) {
> > +                candidate = current;
> > +            }
> > +        }
> > +
> > +        if (curr_log.read < tail) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    *logs = candidate;
> > +
> > +    if (candidate.valid) {
> > +        /* this is the next sequence number, for writes */
> > +        s->log.sequence = candidate.hdr.sequence_number + 1;
> > +    }
> > +
> > +
> > +exit:
> > +    return ret;
> > +}
> > +
> > +/* 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 */
> > +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() */
> > +    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;
> > +    }
> > +
> > +    /* per spec, only log version of 0 is supported */
> > +    if (hdr->log_version != 0) {
> > +        ret = -EINVAL;
> > +        goto exit;
> > +    }
> > +
> > +    /* If either the log guid, or log length is zero,
> > +     * then a replay log is not present */
> > +    if (vhdx_log_guid_is_zero(&hdr->log_guid)) {
> > +        goto exit;
> > +    }
> > +
> > +
> Too many blank lines here.
> > +
> > +    if (hdr->log_length == 0) {
> > +        goto exit;
> > +    }
> > +
> > +    if (hdr->log_length % VHDX_LOG_MIN_SIZE) {
> > +        ret = -EINVAL;
> > +        goto exit;
> > +    }
> > +
> > +
> > +    /* The log is present, we need to find if and where there is an active
> > +     * sequence of valid entries present in the log.  */
> > +
> > +    ret = vhdx_log_search(bs, s, &logs);
> > +    if (ret < 0) {
> > +        goto exit;
> > +    }
> > +
> > +    if (logs.valid) {
> > +        /* now flush the log */
> > +        ret = vhdx_log_flush(bs, s, &logs);
> 
> Does it flush log regardless of read-only open?
> 

Yes - that will cause an error return to go up the stack once the
write is attempted.

Alternatively, I could return error if the image file has a dirty log,
and is opened read-only (in that case, could return -ENOTSUP).

> > +    }
> > +
> > +
> > +exit:
> > +    return ret;
> > +}
> > +
> > diff --git a/block/vhdx.c b/block/vhdx.c
> > index f5689c3..a8dd6d7 100644
> > --- a/block/vhdx.c
> > +++ b/block/vhdx.c
> > @@ -735,48 +735,6 @@ exit:
> >      return ret;
> >  }
> >  
> > -/* 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 */
> > -static int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s)
> > -{
> > -    int ret = 0;
> > -    int i;
> > -    VHDXHeader *hdr;
> > -
> > -    hdr = s->headers[s->curr_header];
> > -
> > -    /* either the log guid, or log length is zero,
> > -     * then a replay log is present */
> > -    for (i = 0; i < sizeof(hdr->log_guid.data4); i++) {
> > -        ret |= hdr->log_guid.data4[i];
> > -    }
> > -    if (hdr->log_guid.data1 == 0 &&
> > -        hdr->log_guid.data2 == 0 &&
> > -        hdr->log_guid.data3 == 0 &&
> > -        ret == 0) {
> > -        goto exit;
> > -    }
> > -
> > -    /* per spec, only log version of 0 is supported */
> > -    if (hdr->log_version != 0) {
> > -        ret = -EINVAL;
> > -        goto exit;
> > -    }
> > -
> > -    if (hdr->log_length == 0) {
> > -        goto exit;
> > -    }
> > -
> > -    /* We currently do not support images with logs to replay */
> > -    ret = -ENOTSUP;
> > -
> > -exit:
> > -    return ret;
> > -}
> > -
> >  
> >  static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
> >  {
> > @@ -789,6 +747,7 @@ static int vhdx_open(BlockDriverState *bs, QDict 
> > *options, int flags)
> >  
> >      s->bat = NULL;
> >      s->first_visible_write = true;
> > +    s->log.write = s->log.read = 0;
> >  
> >      qemu_co_mutex_init(&s->lock);
> >  
> > @@ -1030,6 +989,7 @@ static void vhdx_close(BlockDriverState *bs)
> >      qemu_vfree(s->headers[1]);
> >      qemu_vfree(s->bat);
> >      qemu_vfree(s->parent_entries);
> > +    qemu_vfree(s->log.hdr);
> >  }
> >  
> >  static BlockDriver bdrv_vhdx = {
> > diff --git a/block/vhdx.h b/block/vhdx.h
> > index cb3ce0e..24b126e 100644
> > --- a/block/vhdx.h
> > +++ b/block/vhdx.h
> > @@ -326,7 +326,11 @@ typedef struct VHDXMetadataEntries {
> >  typedef struct VHDXLogEntries {
> >      uint64_t offset;
> >      uint64_t length;
> > -    uint32_t head;
> > +    uint32_t write;
> > +    uint32_t read;
> > +    VHDXLogEntryHeader *hdr;
> > +    void *desc_buffer;
> > +    uint64_t sequence;
> >      uint32_t tail;
> >  } VHDXLogEntries;
> >  
> > @@ -387,6 +391,7 @@ uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, 
> > size_t size,
> >  
> >  bool vhdx_checksum_is_valid(uint8_t *buf, size_t size, int crc_offset);
> >  
> > +int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s);
> >  
> >  static inline void leguid_to_cpus(MSGUID *guid)
> >  {
> > -- 
> > 1.8.1.4
> > 
> > 
> 
> -- 
> Fam