From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KQkYL-00085u-ED for qemu-devel@nongnu.org; Wed, 06 Aug 2008 11:03:53 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KQkYJ-00082K-Rn for qemu-devel@nongnu.org; Wed, 06 Aug 2008 11:03:52 -0400 Received: from [199.232.76.173] (port=51153 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KQkYJ-00081k-C6 for qemu-devel@nongnu.org; Wed, 06 Aug 2008 11:03:51 -0400 Received: from ecfrec.frec.bull.fr ([129.183.4.8]:38392) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KQkYI-0002OE-9Z for qemu-devel@nongnu.org; Wed, 06 Aug 2008 11:03:51 -0400 Subject: Re: [Qemu-devel] [patch 3/5][v2] Extract compressing part from alloc_cluster_offset() From: Laurent Vivier In-Reply-To: <4899BB8F.80602@suse.de> References: <20080729141352.573798859@bull.net> <20080729141447.489521126@bull.net> <4899B321.3060904@suse.de> <1218033682.3964.19.camel@frecb07144> <4899BB8F.80602@suse.de> Content-Type: text/plain; charset=utf-8 Date: Wed, 06 Aug 2008 17:03:44 +0200 Message-Id: <1218035024.3964.23.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:56 +0200, Kevin Wolf a =C3=A9crit= : > Laurent Vivier schrieb: > > Le mercredi 06 ao=C3=BBt 2008 =C3=A0 16:20 +0200, Kevin Wolf a =C3=A9= crit : > >> 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 > >> 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 l= oad > >> 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. >=20 > 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 inter= face. I agree. >=20 > 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(). I agree. > >> Also, the current function name doesn't provide a hint how the retur= n > >> 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 mea= n > >> 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 = the > >=20 > > No, at the end of function the cluster has been freed and the offset = is > > not valid anymore. >=20 > Yes, you have a point there... >=20 > >> 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 sur= e > >> 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 sa= y > > that offset cannot be 0, but again I keep the original behavior. >=20 > I fully agree if you want to keep the original behaviour in the externa= l > interface. However, free_used_clusters() is only used in your own > functions, so you could actually make that change without a too big ris= k. I'll try. No promise... > But the most important part is really the documentation. You see how I > misunderstood what you were trying to achieve. This could be easily avo= ided. OK. Thank you for your comments, 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