From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKr1N-0003Xf-AF for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:45:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKr1G-0005mq-RM for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:44:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33922) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKr1G-0005md-KI for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:44:50 -0400 Message-ID: <53F76566.2040003@redhat.com> Date: Fri, 22 Aug 2014 17:44:38 +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> <53F76135.4010703@redhat.com> <20140822153729.GA9526@nodalink.com> In-Reply-To: <20140822153729.GA9526@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?= Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 22.08.2014 17:37, Beno=EEt Canet wrote: > On Fri, Aug 22, 2014 at 05:26:45PM +0200, Max Reitz wrote: >> 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 = check >>>>>> into an own function. >>>>>> >>>>>> Also, do not use g_realloc() for increasing the size of the in-mem= ory >>>>>> refcount table, but rather g_try_realloc(). >>>> The "Also," may be a sign of doing two things in one patch that coul= d >>>> have been two. >> The second change fit really well into this patch, so I left it in. >> On the other hand, I just noticed I don't need to do that at all, >> because 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 to= taly confuse >>>>> git blame the day we will need it. >>>> I disagree. When refactoring to split a large function into multipl= e >>>> 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 wil= l >>>> 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 i= nto their own >>> patches. >>> This way the extraction patch would be cleaner (no code disapearing a= nd appearing >>> elsewere with another author) and the operations could be reviewed mo= re easily >>> with the various code review tools. >> The main cause for the diff cluttering is making nb_clusters and >> refcount_table pointers in the function. Unfortunately (or maybe >> fortunately?) this is not C++, so I cannot make them references, but >> they need to be passed by reference. The function absolutely must be >> able to change their values and I don't see any way of avoiding this >> even in the first extraction patch. > Do a: git show f75c744 | kompare - > > You will see a whole block of code being morphed to int64_t i and the s= ame whole > block reapear below. > I think this is a consequence of the extracted functions moving up at t= he same time > they are extracted. This is not a consequence of the extracted functions moving up, but=20 because of check_refblocks() preceding calculate_refcounts(). Splitting=20 the check won't help; and I know that Eric dislikes=20 (forward-)declarations of static functions (which I have to do if I pull=20 calculate_refcounts() before check_refblocks()). Max