From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58944) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKquI-0006ne-Aq for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:37:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKquC-0003lY-4l for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:37:38 -0400 Received: from dew.nodalink.com ([95.130.14.197]:39885) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKquB-0003lU-Qd for qemu-devel@nongnu.org; Fri, 22 Aug 2014 11:37:32 -0400 Date: Fri, 22 Aug 2014 15:37:29 +0000 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140822153729.GA9526@nodalink.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <53F76135.4010703@redhat.com> 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: Max Reitz Cc: Kevin Wolf , Stefan Hajnoczi , qemu-devel@nongnu.org, =?iso-8859-1?Q?Beno=EEt?= Canet 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 c= heck > >>>>into an own function. > >>>> > >>>>Also, do not use g_realloc() for increasing the size of the in-memo= ry > >>>>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. >=20 > 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. >=20 > >>>>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 tot= aly 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 in= to their own > >patches. > >This way the extraction patch would be cleaner (no code disapearing an= d appearing > >elsewere with another author) and the operations could be reviewed mor= e easily > >with the various code review tools. >=20 > 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 sam= e whole block reapear below. I think this is a consequence of the extracted functions moving up at the= same time they are extracted. Best regards Beno=EEt >=20 > I'll do my best on the rest, but the diff not being trivial is > mainly simply due to the movement not being absolutely trivial; all > accesses to nb_clusters and refcount_table have to be modified. >=20 > Max