Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check

[Max Reitz]

On 21.08.2014 23:16, Benoît Canet wrote:
> On Thu, Aug 21, 2014 at 01:13:20PM -0600, Eric Blake wrote:
> > On 08/21/2014 12:57 PM, Benoît 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-memory
> > > > 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 
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 <address@hidden>
> > > > ---
> > > >   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 totaly 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 into 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 
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.

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.

Max