From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:55831)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1TiPhP-0004sY-C9
	for qemu-devel@nongnu.org; Tue, 11 Dec 2012 08:16:45 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1TiPhJ-0004A6-Ep
	for qemu-devel@nongnu.org; Tue, 11 Dec 2012 08:16:39 -0500
Received: from mail-wi0-f177.google.com ([209.85.212.177]:45850)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1TiPhJ-0004A1-87
	for qemu-devel@nongnu.org; Tue, 11 Dec 2012 08:16:33 -0500
Received: by mail-wi0-f177.google.com with SMTP id hm2so1757704wib.10
	for <qemu-devel@nongnu.org>; Tue, 11 Dec 2012 05:16:31 -0800 (PST)
Date: Tue, 11 Dec 2012 14:16:28 +0100
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20121211131628.GK796@stefanha-thinkpad.muc.redhat.com>
References: <1353935123-24199-1-git-send-email-benoit@irqsave.net>
	<1353935123-24199-7-git-send-email-benoit@irqsave.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1353935123-24199-7-git-send-email-benoit@irqsave.net>
Subject: Re: [Qemu-devel] [RFC V3 06/24] qcow2: Add qcow2_dedup and related
 functions.
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: =?iso-8859-1?Q?Beno=EEt?= Canet <benoit@irqsave.net>
Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.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.