From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56692) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKqk5-0000lY-9d for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:27:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKqjv-0000fm-S5 for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:27:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55507) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKqjv-0000fc-Ko for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:26:55 -0400 Message-ID: <53F76135.4010703@redhat.com> Date: Fri, 22 Aug 2014 17:26:45 +0200 From: Max Reitz MIME-Version: 1.0 References: <1408115786-13640-1-git-send-email-mreitz@redhat.com> <1408115786-13640-3-git-send-email-mreitz@redhat.com> <20140821185737.GB15636@nodalink.com> <53F644D0.2020701@redhat.com> <20140821211607.GC15636@nodalink.com> In-Reply-To: <20140821211607.GC15636@nodalink.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?windows-1252?Q?Beno=EEt_Canet?= , Eric Blake Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 21.08.2014 23:16, Beno=EEt Canet wrote: > On Thu, Aug 21, 2014 at 01:13:20PM -0600, Eric Blake wrote: >> On 08/21/2014 12:57 PM, Beno=EEt Canet wrote: >>> On Fri, Aug 15, 2014 at 05:16:19PM +0200, Max Reitz wrote: >>>> Put the code for calculating the reference counts during qemu-img ch= eck >>>> into an own function. >>>> >>>> Also, do not use g_realloc() for increasing the size of the in-memor= y >>>> refcount table, but rather g_try_realloc(). >> The "Also," may be a sign of doing two things in one patch that could >> have been two. The second change fit really well into this patch, so I left it in. On=20 the other hand, I just noticed I don't need to do that at all, because=20 that call will be dropped in patch 5 anyway. >>>> Signed-off-by: Max Reitz >>>> --- >>>> block/qcow2-refcount.c | 188 ++++++++++++++++++++++++++++++-------= ------------ >>>> 1 file changed, 115 insertions(+), 73 deletions(-) >> But overall, this patch seems like a reasonable refactor to me, >> splitting out a reusable piece. >> >>> Another point is that I think these two extractions patches will tota= ly confuse >>> git blame the day we will need it. >> I disagree. When refactoring to split a large function into multiple >> smaller functions, where the original function calls out to a helper >> function for what it used to do inline, git blame tracks things >> perfectly. Someone following blame may end up on this commit as the >> source of a given line in its new location, but the blame viewer will >> also show this commit is a refactor, and it should be fairly easy to >> find where the line was moved from, and resume blaming even further. > I think we could separate the extractions and the respectives moves int= o their own > patches. > This way the extraction patch would be cleaner (no code disapearing and= appearing > elsewere with another author) and the operations could be reviewed more= easily > with the various code review tools. The main cause for the diff cluttering is making nb_clusters and=20 refcount_table pointers in the function. Unfortunately (or maybe=20 fortunately?) this is not C++, so I cannot make them references, but=20 they need to be passed by reference. The function absolutely must be=20 able to change their values and I don't see any way of avoiding this=20 even in the first extraction patch. I'll do my best on the rest, but the diff not being trivial is mainly=20 simply due to the movement not being absolutely trivial; all accesses to=20 nb_clusters and refcount_table have to be modified. Max