[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting fo
From: |
Benoît Canet |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check |
Date: |
Fri, 22 Aug 2014 15:37:29 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Aug 22, 2014 at 05:26:45PM +0200, Max Reitz wrote:
> 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.
Do a: git show f75c744 | kompare -
You will see a whole block of code being morphed to int64_t i and the same 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ît
>
> 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
- [Qemu-devel] [PATCH v2 0/9] qcow2: Fix image repairing, Max Reitz, 2014/08/15
- [Qemu-devel] [PATCH v2 1/9] qcow2: Fix leaks in dirty images, Max Reitz, 2014/08/15
- [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Max Reitz, 2014/08/15
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Benoît Canet, 2014/08/21
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Benoît Canet, 2014/08/21
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Eric Blake, 2014/08/21
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Benoît Canet, 2014/08/21
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Max Reitz, 2014/08/22
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check,
Benoît Canet <=
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Max Reitz, 2014/08/22
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Max Reitz, 2014/08/22
- Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check, Eric Blake, 2014/08/22
[Qemu-devel] [PATCH v2 3/9] qcow2: Factor out refcount comparison for check, Max Reitz, 2014/08/15
[Qemu-devel] [PATCH v2 4/9] qcow2: Fix refcount blocks beyond image end, Max Reitz, 2014/08/15
[Qemu-devel] [PATCH v2 5/9] qcow2: Do not perform potentially damaging repairs, Max Reitz, 2014/08/15
[Qemu-devel] [PATCH v2 6/9] qcow2: Rebuild refcount structure during check, Max Reitz, 2014/08/15
[Qemu-devel] [PATCH v2 8/9] iotests: Fix test outputs, Max Reitz, 2014/08/15
[Qemu-devel] [PATCH v2 9/9] iotests: Add test for potentially damaging repairs, Max Reitz, 2014/08/15
[Qemu-devel] [PATCH v2 7/9] qcow2: Clean up after refcount rebuild, Max Reitz, 2014/08/15