From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1KQjtQ-00088e-Vx
	for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:21:37 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1KQjtQ-00087P-0H
	for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:21:36 -0400
Received: from [199.232.76.173] (port=58183 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1KQjtP-000879-Nw
	for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:21:35 -0400
Received: from mx2.suse.de ([195.135.220.15]:53130)
	by monty-python.gnu.org with esmtps
	(TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60)
	(envelope-from <kwolf@suse.de>) id 1KQjtP-0002fn-G2
	for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:21:35 -0400
Message-ID: <4899B321.3060904@suse.de>
Date: Wed, 06 Aug 2008 16:20:17 +0200
From: Kevin Wolf <kwolf@suse.de>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [patch 3/5][v2] Extract compressing part
	from	alloc_cluster_offset()
References: <20080729141352.573798859@bull.net>
	<20080729141447.489521126@bull.net>
In-Reply-To: <20080729141447.489521126@bull.net>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Laurent Vivier <Laurent.Vivier@bull.net>
Cc: qemu-devel@nongnu.org

Laurent Vivier schrieb:
> Divide alloc_cluster_offset() into alloc_cluster_offset() and
> alloc_compressed_cluster_offset(). Factorize code to free clusters into
> free_used_clusters().
> 
> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>

I think free_used_clusters() is misnamed. That it frees clusters is more
of an additional action it performs. What it really is doing is to load
the appropriate L2 table into memory (and to allocate it if needed).

Also, the current function name doesn't provide a hint how the return
value is to be interpreted. If I'm not mistaken, 0 could mean that
everything went fine and the cluster has been freed, or it could mean
that an error occurred.

Maybe you meant to return cluster_offset instead of 0 at the end of the
function (that you check for QCOW_OFLAG_COPIED which is _always_ set if
the function returns != 0 suggests this), but I can't tell for sure
because there is no documentation on the return value.

So I suggest that, besides renaming and possibly fixing, you add a
header comment to the function which describes the whole functionality
it performs (it's too much to fit in a function name) and what the
return value actually means.

Kevin