Re: [Qemu-devel] [RFC V3 06/24] qcow2: Add qcow2_dedup and related functions.

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, Nov 26, 2012 at 02:05:05PM +0100, Benoît Canet wrote:
> +/*
> + * Compute the hash of a given cluster
> + *
> + * @data: a buffer containing the cluster data
> + * @ret:  a HASH_LENGTH long dynamically allocated array containing the hash
> + */
> +static uint8_t *qcow2_compute_cluster_hash(BlockDriverState *bs,
> +                                           uint8_t *data)
> +{
> +    return NULL;
> +}
> +
> +/* Try to find the offset of a given cluster if it's duplicated
> + * Exceptionally we cast return value to int64_t to use as error code.

I don't see an int64_t return value, this comment is probably outdated.

> + *
> + * @data:            a buffer containing the cluster
> + * @skip_cluster_nr: the number of cluster to skip in the buffer
> + * @hash:            if hash is provided it's used else it's computed
> + * @ret:             QCowHashNode of the duplicated cluster or NULL

This function does several things:
1. Allocating and computing *hash if not given.
2. Returning existing dedup_tree_by_hash node or NULL if the node wasn't
   already in the tree.
3. Inserting the node into the tree if not present.

I wonder if it can be simplified or split to do less work.

> +/*
> + * Helper used to link a deduplicated cluster in the l2
> + *
> + * @logical_cluster_offset:  the cluster offset seen by the guest (in sectors)
> + * @physical_cluster_offset: the cluster offset in the QCOW2 file (in sectors)

Perhaps s/_offset/_sect/ because usually offset is in bytes.