[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: |
Thu, 21 Aug 2014 21:16:07 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
>
> >>
> >> 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.
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
- [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 <=
- 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, 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, 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