From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KQkDE-0000du-0c for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:42:04 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KQkDC-0000dY-1c for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:42:03 -0400 Received: from [199.232.76.173] (port=58213 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KQkDB-0000dV-Ob for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:42:01 -0400 Received: from ecfrec.frec.bull.fr ([129.183.4.8]:56847) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KQkDB-00067Z-79 for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:42:01 -0400 Subject: Re: [Qemu-devel] [patch 3/5][v2] Extract compressing part from alloc_cluster_offset() From: Laurent Vivier In-Reply-To: <4899B321.3060904@suse.de> References: <20080729141352.573798859@bull.net> <20080729141447.489521126@bull.net> <4899B321.3060904@suse.de> Content-Type: text/plain; charset=utf-8 Date: Wed, 06 Aug 2008 16:41:22 +0200 Message-Id: <1218033682.3964.19.camel@frecb07144> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org Le mercredi 06 ao=C3=BBt 2008 =C3=A0 16:20 +0200, Kevin Wolf a =C3=A9crit= : > Laurent Vivier schrieb: > > Divide alloc_cluster_offset() into alloc_cluster_offset() and > > alloc_compressed_cluster_offset(). Factorize code to free clusters in= to > > free_used_clusters(). > >=20 > > Signed-off-by: Laurent Vivier >=20 > I think free_used_clusters() is misnamed. That it frees clusters is mor= e > 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). It frees the cluster if it is found except if it has the flag QCOW_OFLAG_COPIED. To do that it needs to load the L2 table. But I'm open to a new name, I don't like this one too. > 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. Yes, but this is the original behavior and I don't want to modify it. >=20 > Maybe you meant to return cluster_offset instead of 0 at the end of the No, at the end of function the cluster has been freed and the offset is not valid anymore. > function (that you check for QCOW_OFLAG_COPIED which is _always_ set if > the function returns !=3D 0 suggests this), but I can't tell for sure > because there is no documentation on the return value. You're right. But to return the offset with the flag allows to differentiate the case returning 0 from an offset equal to 0. If offset is 0, it returns (0 | QCOW_OFLAG_COPIED), but I agree to say that offset cannot be 0, but again I keep the original behavior. > 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. I agree. Could you propose a name ? Regards, Laurent --=20 ----------------- Laurent.Vivier@bull.net ------------------ "La perfection est atteinte non quand il ne reste rien =C3=A0 ajouter mais quand il ne reste rien =C3=A0 enlever." Saint Exup=C3=A9ry