Re: [Qemu-devel] [RFC 18/24] qcow2: add qcow2_map

[Kevin Wolf <kwolf@redhat.com>]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block/qcow2-cluster.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c         |    1 +
>  block/qcow2.h         |    3 +++
>  3 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ca56918..848f2ee 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -977,3 +977,52 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>  
>      return 0;
>  }
> +
> +int qcow2_map(BlockDriverState *bs, uint64_t guest_offset,
> +    uint64_t host_offset, uint64_t contiguous_bytes)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    unsigned int nelms;
> +
> +
> +    if ((s->cluster_size - 1) & guest_offset) {

Usually you would have guest_offset first.

> +        return -EINVAL;
> +    }
> +
> +    if (contiguous_bytes % s->cluster_size) {
> +        return -EINVAL;
> +    }

Any reason why the two checks are different? I don't really care if they
use & or %, but they should use the same thing.

host_offset isn't checked at all?

> +
> +    nelms = s->l2_size / sizeof(uint64_t);

s->l2_size is already in elements, not in bytes.

> +
> +    while (contiguous_bytes > 0) {
> +        unsigned int l1_index, l2_index;
> +        uint64_t *l2_table;
> +        int ret;
> +        l1_index = guest_offset >> (s->l2_bits + s->cluster_bits);
> +        l2_index = (guest_offset  >> s->cluster_bits) & (s->l2_size - 1);
> +
> +        if (!s->l1_table[l1_index]) {
> +            ret = l2_allocate(bs, l1_index, &l2_table);
> +            if (ret) {
> +                return ret;
> +            }
> +        } else {
> +            ret = l2_load(bs, s->l1_table[l1_index] & ~QCOW_OFLAG_COPIED, &l2_table);
> +            if (ret) {
> +                return ret;
> +            }
> +        }

Can't you use get_cluster_table() for this part?

> +        qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
> +
> +        for (; l2_index < nelms && contiguous_bytes > 0;  l2_index++) {
> +            l2_table[l2_index] = cpu_to_be64(host_offset | QCOW_OFLAG_COPIED);

You should increase the refcount for host_offset and set
QCOW_OFLAG_COPIED only if it becomes 1. Or maybe we should just fail
bdrv_map if the old refcount is != 0.

> +            host_offset += s->cluster_size;
> +            contiguous_bytes -= s->cluster_size;
> +        }
> +
> +        qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
> +
> +    }
> +    return 0;
> +}

Kevin