From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KQkSE-0006fW-Hq for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:57:34 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KQkSD-0006dt-Mg for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:57:33 -0400 Received: from [199.232.76.173] (port=57618 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KQkSD-0006dX-8p for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:57:33 -0400 Received: from ns.suse.de ([195.135.220.2]:43558 helo=mx1.suse.de) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1KQkSC-0001BV-Mc for qemu-devel@nongnu.org; Wed, 06 Aug 2008 10:57:33 -0400 Message-ID: <4899BB8F.80602@suse.de> Date: Wed, 06 Aug 2008 16:56:15 +0200 From: Kevin Wolf 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> <4899B321.3060904@suse.de> <1218033682.3964.19.camel@frecb07144> In-Reply-To: <1218033682.3964.19.camel@frecb07144> Content-Type: text/plain; charset=UTF-8 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: Laurent Vivier Cc: qemu-devel@nongnu.org Laurent Vivier schrieb: > Le mercredi 06 ao=C3=BBt 2008 =C3=A0 16:20 +0200, Kevin Wolf a =C3=A9cr= it : >> 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(). >>> >>> Signed-off-by: Laurent Vivier >> I think free_used_clusters() is misnamed. That it frees clusters is mo= re >> of an additional action it performs. What it really is doing is to loa= d >> the appropriate L2 table into memory (and to allocate it if needed). >=20 > 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. >=20 > But I'm open to a new name, I don't like this one too. Well, yes and no. Of course, it needs to have the L2 table to free the cluster. But it does not only load that table for local usage but returns it to the caller using the parameters passed by reference. So this loading doesn't only happen internally but it belongs to the interfa= ce. Actually, I'm also having a hard time finding a fitting name for the function. Maybe that's an indication that the function is doing two things which don't really belong together. I'm not sure if it makes sense, but you could do the L2 table loading in another function which is called before free_used_clusters() and pass the already loaded table into free_used_clusters(). >> 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. >=20 > 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 th= e >=20 > No, at the end of function the cluster has been freed and the offset is > not valid anymore. Yes, you have a point there... >> function (that you check for QCOW_OFLAG_COPIED which is _always_ set i= f >> the function returns !=3D 0 suggests this), but I can't tell for sure >> because there is no documentation on the return value. >=20 > 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. I fully agree if you want to keep the original behaviour in the external interface. However, free_used_clusters() is only used in your own functions, so you could actually make that change without a too big risk. But the most important part is really the documentation. You see how I misunderstood what you were trying to achieve. This could be easily avoid= ed. Kevin