Re: [Qemu-devel] [PATCH V17 04/12] blkverify: Extract qemu_iovec_clone() and qemu_iovec_compare() from blkverify.

[Max Reitz <mreitz@redhat.com>]

On 12.02.2014 23:06, Benoît Canet wrote:
> From: Benoît Canet <benoit@irqsave.net>
>
> qemu_iovec_compare() will be used to compare IOs vectors in quorum blkverify
> mode. The patch extract these functions in order to factorize the code.

"extracts" (just a sidenode: If I'm correcting (or thinking I'm 
correcting) spelling mistakes, I'm just saying "Try to fix it if you're 
doing a new version anyway", but I won't force you to; but if you don't 
correct it, prepare for me pointing it out again ;-))

Reviewed-by: Max Reitz <mreitz@redhat.com>

> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/blkverify.c     | 108 +-------------------------------------------------
>   include/qemu-common.h |   2 +
>   util/iov.c            | 106 +++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 110 insertions(+), 106 deletions(-)
>
> diff --git a/block/blkverify.c b/block/blkverify.c
> index b57da59..c86ad7a 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -173,110 +173,6 @@ static int64_t blkverify_getlength(BlockDriverState *bs)
>       return bdrv_getlength(s->test_file);
>   }
>   
> -/**
> - * Check that I/O vector contents are identical
> - *
> - * @a:          I/O vector
> - * @b:          I/O vector
> - * @ret:        Offset to first mismatching byte or -1 if match
> - */
> -static ssize_t blkverify_iovec_compare(QEMUIOVector *a, QEMUIOVector *b)
> -{
> -    int i;
> -    ssize_t offset = 0;
> -
> -    assert(a->niov == b->niov);
> -    for (i = 0; i < a->niov; i++) {
> -        size_t len = 0;
> -        uint8_t *p = (uint8_t *)a->iov[i].iov_base;
> -        uint8_t *q = (uint8_t *)b->iov[i].iov_base;
> -
> -        assert(a->iov[i].iov_len == b->iov[i].iov_len);
> -        while (len < a->iov[i].iov_len && *p++ == *q++) {
> -            len++;
> -        }
> -
> -        offset += len;
> -
> -        if (len != a->iov[i].iov_len) {
> -            return offset;
> -        }
> -    }
> -    return -1;
> -}
> -
> -typedef struct {
> -    int src_index;
> -    struct iovec *src_iov;
> -    void *dest_base;
> -} IOVectorSortElem;
> -
> -static int sortelem_cmp_src_base(const void *a, const void *b)
> -{
> -    const IOVectorSortElem *elem_a = a;
> -    const IOVectorSortElem *elem_b = b;
> -
> -    /* Don't overflow */
> -    if (elem_a->src_iov->iov_base < elem_b->src_iov->iov_base) {
> -        return -1;
> -    } else if (elem_a->src_iov->iov_base > elem_b->src_iov->iov_base) {
> -        return 1;
> -    } else {
> -        return 0;
> -    }
> -}
> -
> -static int sortelem_cmp_src_index(const void *a, const void *b)
> -{
> -    const IOVectorSortElem *elem_a = a;
> -    const IOVectorSortElem *elem_b = b;
> -
> -    return elem_a->src_index - elem_b->src_index;
> -}
> -
> -/**
> - * Copy contents of I/O vector
> - *
> - * The relative relationships of overlapping iovecs are preserved.  This is
> - * necessary to ensure identical semantics in the cloned I/O vector.
> - */
> -static void blkverify_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src,
> -                                  void *buf)
> -{
> -    IOVectorSortElem sortelems[src->niov];
> -    void *last_end;
> -    int i;
> -
> -    /* Sort by source iovecs by base address */
> -    for (i = 0; i < src->niov; i++) {
> -        sortelems[i].src_index = i;
> -        sortelems[i].src_iov = &src->iov[i];
> -    }
> -    qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_base);
> -
> -    /* Allocate buffer space taking into account overlapping iovecs */
> -    last_end = NULL;
> -    for (i = 0; i < src->niov; i++) {
> -        struct iovec *cur = sortelems[i].src_iov;
> -        ptrdiff_t rewind = 0;
> -
> -        /* Detect overlap */
> -        if (last_end && last_end > cur->iov_base) {
> -            rewind = last_end - cur->iov_base;
> -        }
> -
> -        sortelems[i].dest_base = buf - rewind;
> -        buf += cur->iov_len - MIN(rewind, cur->iov_len);
> -        last_end = MAX(cur->iov_base + cur->iov_len, last_end);
> -    }
> -
> -    /* Sort by source iovec index and build destination iovec */
> -    qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_index);
> -    for (i = 0; i < src->niov; i++) {
> -        qemu_iovec_add(dest, sortelems[i].dest_base, src->iov[i].iov_len);
> -    }
> -}
> -
>   static BlkverifyAIOCB *blkverify_aio_get(BlockDriverState *bs, bool is_write,
>                                            int64_t sector_num, QEMUIOVector *qiov,
>                                            int nb_sectors,
> @@ -340,7 +236,7 @@ static void blkverify_aio_cb(void *opaque, int ret)
>   
>   static void blkverify_verify_readv(BlkverifyAIOCB *acb)
>   {
> -    ssize_t offset = blkverify_iovec_compare(acb->qiov, &acb->raw_qiov);
> +    ssize_t offset = qemu_iovec_compare(acb->qiov, &acb->raw_qiov);
>       if (offset != -1) {
>           blkverify_err(acb, "contents mismatch in sector %" PRId64,
>                         acb->sector_num + (int64_t)(offset / BDRV_SECTOR_SIZE));
> @@ -358,7 +254,7 @@ static BlockDriverAIOCB *blkverify_aio_readv(BlockDriverState *bs,
>       acb->verify = blkverify_verify_readv;
>       acb->buf = qemu_blockalign(bs->file, qiov->size);
>       qemu_iovec_init(&acb->raw_qiov, acb->qiov->niov);
> -    blkverify_iovec_clone(&acb->raw_qiov, qiov, acb->buf);
> +    qemu_iovec_clone(&acb->raw_qiov, qiov, acb->buf);
>   
>       bdrv_aio_readv(s->test_file, sector_num, qiov, nb_sectors,
>                      blkverify_aio_cb, acb);
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 5054836..d70783e 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -346,6 +346,8 @@ size_t qemu_iovec_from_buf(QEMUIOVector *qiov, size_t offset,
>                              const void *buf, size_t bytes);
>   size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
>                            int fillc, size_t bytes);
> +ssize_t qemu_iovec_compare(QEMUIOVector *a, QEMUIOVector *b);
> +void qemu_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src, void *buf);
>   
>   bool buffer_is_zero(const void *buf, size_t len);
>   
> diff --git a/util/iov.c b/util/iov.c
> index bb46c04..03934da 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -378,6 +378,112 @@ size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset,
>       return iov_memset(qiov->iov, qiov->niov, offset, fillc, bytes);
>   }
>   
> +/**
> + * Check that I/O vector contents are identical
> + *
> + * The IO vectors must have the same structure (same length of all parts).
> + * A typical usage is to compare vectors created with qemu_iovec_clone().
> + *
> + * @a:          I/O vector
> + * @b:          I/O vector
> + * @ret:        Offset to first mismatching byte or -1 if match
> + */
> +ssize_t qemu_iovec_compare(QEMUIOVector *a, QEMUIOVector *b)
> +{
> +    int i;
> +    ssize_t offset = 0;
> +
> +    assert(a->niov == b->niov);
> +    for (i = 0; i < a->niov; i++) {
> +        size_t len = 0;
> +        uint8_t *p = (uint8_t *)a->iov[i].iov_base;
> +        uint8_t *q = (uint8_t *)b->iov[i].iov_base;
> +
> +        assert(a->iov[i].iov_len == b->iov[i].iov_len);
> +        while (len < a->iov[i].iov_len && *p++ == *q++) {
> +            len++;
> +        }
> +
> +        offset += len;
> +
> +        if (len != a->iov[i].iov_len) {
> +            return offset;
> +        }
> +    }
> +    return -1;
> +}
> +
> +typedef struct {
> +    int src_index;
> +    struct iovec *src_iov;
> +    void *dest_base;
> +} IOVectorSortElem;
> +
> +static int sortelem_cmp_src_base(const void *a, const void *b)
> +{
> +    const IOVectorSortElem *elem_a = a;
> +    const IOVectorSortElem *elem_b = b;
> +
> +    /* Don't overflow */
> +    if (elem_a->src_iov->iov_base < elem_b->src_iov->iov_base) {
> +        return -1;
> +    } else if (elem_a->src_iov->iov_base > elem_b->src_iov->iov_base) {
> +        return 1;
> +    } else {
> +        return 0;
> +    }
> +}
> +
> +static int sortelem_cmp_src_index(const void *a, const void *b)
> +{
> +    const IOVectorSortElem *elem_a = a;
> +    const IOVectorSortElem *elem_b = b;
> +
> +    return elem_a->src_index - elem_b->src_index;
> +}
> +
> +/**
> + * Copy contents of I/O vector
> + *
> + * The relative relationships of overlapping iovecs are preserved.  This is
> + * necessary to ensure identical semantics in the cloned I/O vector.
> + */
> +void qemu_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src, void *buf)
> +{
> +    IOVectorSortElem sortelems[src->niov];
> +    void *last_end;
> +    int i;
> +
> +    /* Sort by source iovecs by base address */
> +    for (i = 0; i < src->niov; i++) {
> +        sortelems[i].src_index = i;
> +        sortelems[i].src_iov = &src->iov[i];
> +    }
> +    qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_base);
> +
> +    /* Allocate buffer space taking into account overlapping iovecs */
> +    last_end = NULL;
> +    for (i = 0; i < src->niov; i++) {
> +        struct iovec *cur = sortelems[i].src_iov;
> +        ptrdiff_t rewind = 0;
> +
> +        /* Detect overlap */
> +        if (last_end && last_end > cur->iov_base) {
> +            rewind = last_end - cur->iov_base;
> +        }
> +
> +        sortelems[i].dest_base = buf - rewind;
> +        buf += cur->iov_len - MIN(rewind, cur->iov_len);
> +        last_end = MAX(cur->iov_base + cur->iov_len, last_end);
> +    }
> +
> +    /* Sort by source iovec index and build destination iovec */
> +    qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_index);
> +    for (i = 0; i < src->niov; i++) {
> +        qemu_iovec_add(dest, sortelems[i].dest_base, src->iov[i].iov_len);
> +    }
> +}
> +
>   size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
>                            size_t bytes)
>   {