| Message ID | 1408115786-13640-3-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-refcount.c | 188 ++++++++++++++++++++++++++++++------------------- > 1 file changed, 115 insertions(+), 73 deletions(-) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index d60e2fe..b3c4edd 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1496,69 +1496,15 @@ done: > } > > /* > - * Checks an image for refcount consistency. > - * > - * Returns 0 if no errors are found, the number of errors in case the image is > - * detected as corrupted, and -errno when an internal error occurred. > + * Checks consistency of refblocks and accounts for each refblock in > + * *refcount_table. > */ > -int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > - BdrvCheckMode fix) > +static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res, > + BdrvCheckMode fix, uint16_t **refcount_table, > + int64_t *nb_clusters) > { > BDRVQcowState *s = bs->opaque; > - int64_t size, i, highest_cluster, nb_clusters; > - int refcount1, refcount2; > - QCowSnapshot *sn; > - uint16_t *refcount_table; > - int ret; > - > - size = bdrv_getlength(bs->file); > - if (size < 0) { > - res->check_errors++; > - return size; > - } > - > - nb_clusters = size_to_clusters(s, size); > - if (nb_clusters > INT_MAX) { > - res->check_errors++; > - return -EFBIG; > - } > - > - refcount_table = g_try_malloc0(nb_clusters * sizeof(uint16_t)); > - if (nb_clusters && refcount_table == NULL) { > - res->check_errors++; > - return -ENOMEM; > - } > - > - res->bfi.total_clusters = > - size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); > - > - /* header */ > - inc_refcounts(bs, res, refcount_table, nb_clusters, > - 0, s->cluster_size); > - > - /* current L1 table */ > - ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters, > - s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO); > - if (ret < 0) { > - goto fail; > - } > - > - /* snapshots */ > - for(i = 0; i < s->nb_snapshots; i++) { > - sn = s->snapshots + i; > - ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters, > - sn->l1_table_offset, sn->l1_size, 0); > - if (ret < 0) { > - goto fail; > - } > - } > - inc_refcounts(bs, res, refcount_table, nb_clusters, > - s->snapshots_offset, s->snapshots_size); > - > - /* refcount data */ > - inc_refcounts(bs, res, refcount_table, nb_clusters, > - s->refcount_table_offset, > - s->refcount_table_size * sizeof(uint64_t)); > + int64_t i; > > for(i = 0; i < s->refcount_table_size; i++) { > uint64_t offset, cluster; > @@ -1573,7 +1519,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > continue; > } > > - if (cluster >= nb_clusters) { > + if (cluster >= *nb_clusters) { > fprintf(stderr, "ERROR refcount block %" PRId64 > " is outside image\n", i); > res->corruptions++; > @@ -1581,14 +1527,14 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > } > > if (offset != 0) { > - inc_refcounts(bs, res, refcount_table, nb_clusters, > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > offset, s->cluster_size); > - if (refcount_table[cluster] != 1) { > + if ((*refcount_table)[cluster] != 1) { > fprintf(stderr, "%s refcount block %" PRId64 > " refcount=%d\n", > fix & BDRV_FIX_ERRORS ? "Repairing" : > "ERROR", > - i, refcount_table[cluster]); > + i, (*refcount_table)[cluster]); > > if (fix & BDRV_FIX_ERRORS) { > int64_t new_offset; > @@ -1600,17 +1546,24 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > } > > /* update refcounts */ > - if ((new_offset >> s->cluster_bits) >= nb_clusters) { > + if ((new_offset >> s->cluster_bits) >= *nb_clusters) { > /* increase refcount_table size if necessary */ > - int old_nb_clusters = nb_clusters; > - nb_clusters = (new_offset >> s->cluster_bits) + 1; > - refcount_table = g_realloc(refcount_table, > - nb_clusters * sizeof(uint16_t)); > - memset(&refcount_table[old_nb_clusters], 0, (nb_clusters > - - old_nb_clusters) * sizeof(uint16_t)); > + int old_nb_clusters = *nb_clusters; > + *nb_clusters = (new_offset >> s->cluster_bits) + 1; > + > + *refcount_table = g_try_realloc(*refcount_table, > + *nb_clusters * sizeof(uint16_t)); > + if (!*refcount_table) { > + res->check_errors++; > + return -ENOMEM; > + } > + > + memset(&(*refcount_table)[old_nb_clusters], 0, > + (*nb_clusters - old_nb_clusters) * > + sizeof(uint16_t)); > } > - refcount_table[cluster]--; > - inc_refcounts(bs, res, refcount_table, nb_clusters, > + (*refcount_table)[cluster]--; > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > new_offset, s->cluster_size); > > res->corruptions_fixed++; > @@ -1621,6 +1574,95 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > } > } > > + return 0; > +} > + > +/* > + * Calculates an in-memory refcount table. > + */ > +static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > + BdrvCheckMode fix, uint16_t **refcount_table, > + int64_t *nb_clusters) > +{ > + BDRVQcowState *s = bs->opaque; > + QCowSnapshot *sn; > + int64_t i; > + int ret; > + > + if (!*refcount_table) { > + *refcount_table = g_try_malloc0(*nb_clusters * sizeof(uint16_t)); > + if (*nb_clusters && !*refcount_table) { > + res->check_errors++; > + return -ENOMEM; > + } > + } > + > + /* header */ > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > + 0, s->cluster_size); > + > + /* current L1 table */ > + ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters, > + s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO); > + if (ret < 0) { > + return ret; > + } > + > + /* snapshots */ > + for (i = 0; i < s->nb_snapshots; i++) { > + sn = s->snapshots + i; > + ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters, > + sn->l1_table_offset, sn->l1_size, 0); > + if (ret < 0) { > + return ret; > + } > + } > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > + s->snapshots_offset, s->snapshots_size); > + > + /* refcount data */ > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > + s->refcount_table_offset, > + s->refcount_table_size * sizeof(uint64_t)); > + > + return check_refblocks(bs, res, fix, refcount_table, nb_clusters); > +} > + > +/* > + * Checks an image for refcount consistency. > + * > + * Returns 0 if no errors are found, the number of errors in case the image is > + * detected as corrupted, and -errno when an internal error occurred. > + */ > +int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > + BdrvCheckMode fix) > +{ > + BDRVQcowState *s = bs->opaque; > + int64_t size, i, highest_cluster, nb_clusters; > + int refcount1, refcount2; > + uint16_t *refcount_table = NULL; > + int ret; > + > + size = bdrv_getlength(bs->file); > + if (size < 0) { > + res->check_errors++; > + return size; > + } > + > + nb_clusters = size_to_clusters(s, size); > + if (nb_clusters > INT_MAX) { > + res->check_errors++; > + return -EFBIG; > + } > + > + res->bfi.total_clusters = > + size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); > + > + ret = calculate_refcounts(bs, res, fix, &refcount_table, &nb_clusters); > + if (ret < 0) { > + goto fail; > + } > + > /* compare ref counts */ > for (i = 0, highest_cluster = 0; i < nb_clusters; i++) { > refcount1 = get_refcount(bs, i); > -- > 2.0.3 > I tried during half an hour to find a git option (--patience etc) or a tool (Kompare) showing me that these code motions are right. The problem is that you extract subfunctions and move them up at the same time. I will try again tomorow to find something making sense of this patch without having to spell check every single character. Best regards Benoît
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(). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-refcount.c | 188 ++++++++++++++++++++++++++++++------------------- > 1 file changed, 115 insertions(+), 73 deletions(-) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index d60e2fe..b3c4edd 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1496,69 +1496,15 @@ done: > } > > /* > - * Checks an image for refcount consistency. > - * > - * Returns 0 if no errors are found, the number of errors in case the image is > - * detected as corrupted, and -errno when an internal error occurred. > + * Checks consistency of refblocks and accounts for each refblock in > + * *refcount_table. > */ > -int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > - BdrvCheckMode fix) > +static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res, > + BdrvCheckMode fix, uint16_t **refcount_table, > + int64_t *nb_clusters) > { > BDRVQcowState *s = bs->opaque; > - int64_t size, i, highest_cluster, nb_clusters; > - int refcount1, refcount2; > - QCowSnapshot *sn; > - uint16_t *refcount_table; > - int ret; > - > - size = bdrv_getlength(bs->file); > - if (size < 0) { > - res->check_errors++; > - return size; > - } > - > - nb_clusters = size_to_clusters(s, size); > - if (nb_clusters > INT_MAX) { > - res->check_errors++; > - return -EFBIG; > - } > - > - refcount_table = g_try_malloc0(nb_clusters * sizeof(uint16_t)); > - if (nb_clusters && refcount_table == NULL) { > - res->check_errors++; > - return -ENOMEM; > - } > - > - res->bfi.total_clusters = > - size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); > - > - /* header */ > - inc_refcounts(bs, res, refcount_table, nb_clusters, > - 0, s->cluster_size); > - > - /* current L1 table */ > - ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters, > - s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO); > - if (ret < 0) { > - goto fail; > - } > - > - /* snapshots */ > - for(i = 0; i < s->nb_snapshots; i++) { > - sn = s->snapshots + i; > - ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters, > - sn->l1_table_offset, sn->l1_size, 0); > - if (ret < 0) { > - goto fail; > - } > - } > - inc_refcounts(bs, res, refcount_table, nb_clusters, > - s->snapshots_offset, s->snapshots_size); > - > - /* refcount data */ > - inc_refcounts(bs, res, refcount_table, nb_clusters, > - s->refcount_table_offset, > - s->refcount_table_size * sizeof(uint64_t)); > + int64_t i; > > for(i = 0; i < s->refcount_table_size; i++) { > uint64_t offset, cluster; > @@ -1573,7 +1519,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > continue; > } > > - if (cluster >= nb_clusters) { > + if (cluster >= *nb_clusters) { > fprintf(stderr, "ERROR refcount block %" PRId64 > " is outside image\n", i); > res->corruptions++; > @@ -1581,14 +1527,14 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > } > > if (offset != 0) { > - inc_refcounts(bs, res, refcount_table, nb_clusters, > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > offset, s->cluster_size); > - if (refcount_table[cluster] != 1) { > + if ((*refcount_table)[cluster] != 1) { > fprintf(stderr, "%s refcount block %" PRId64 > " refcount=%d\n", > fix & BDRV_FIX_ERRORS ? "Repairing" : > "ERROR", > - i, refcount_table[cluster]); > + i, (*refcount_table)[cluster]); > > if (fix & BDRV_FIX_ERRORS) { > int64_t new_offset; > @@ -1600,17 +1546,24 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > } > > /* update refcounts */ > - if ((new_offset >> s->cluster_bits) >= nb_clusters) { > + if ((new_offset >> s->cluster_bits) >= *nb_clusters) { > /* increase refcount_table size if necessary */ > - int old_nb_clusters = nb_clusters; > - nb_clusters = (new_offset >> s->cluster_bits) + 1; > - refcount_table = g_realloc(refcount_table, > - nb_clusters * sizeof(uint16_t)); > - memset(&refcount_table[old_nb_clusters], 0, (nb_clusters > - - old_nb_clusters) * sizeof(uint16_t)); > + int old_nb_clusters = *nb_clusters; > + *nb_clusters = (new_offset >> s->cluster_bits) + 1; > + > + *refcount_table = g_try_realloc(*refcount_table, > + *nb_clusters * sizeof(uint16_t)); > + if (!*refcount_table) { > + res->check_errors++; > + return -ENOMEM; > + } > + > + memset(&(*refcount_table)[old_nb_clusters], 0, > + (*nb_clusters - old_nb_clusters) * > + sizeof(uint16_t)); > } > - refcount_table[cluster]--; > - inc_refcounts(bs, res, refcount_table, nb_clusters, > + (*refcount_table)[cluster]--; > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > new_offset, s->cluster_size); > > res->corruptions_fixed++; > @@ -1621,6 +1574,95 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > } > } > > + return 0; > +} > + > +/* > + * Calculates an in-memory refcount table. > + */ > +static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > + BdrvCheckMode fix, uint16_t **refcount_table, > + int64_t *nb_clusters) > +{ > + BDRVQcowState *s = bs->opaque; > + QCowSnapshot *sn; > + int64_t i; > + int ret; > + > + if (!*refcount_table) { > + *refcount_table = g_try_malloc0(*nb_clusters * sizeof(uint16_t)); > + if (*nb_clusters && !*refcount_table) { > + res->check_errors++; > + return -ENOMEM; > + } > + } > + > + /* header */ > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > + 0, s->cluster_size); > + > + /* current L1 table */ > + ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters, > + s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO); > + if (ret < 0) { > + return ret; > + } > + > + /* snapshots */ > + for (i = 0; i < s->nb_snapshots; i++) { > + sn = s->snapshots + i; > + ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters, > + sn->l1_table_offset, sn->l1_size, 0); > + if (ret < 0) { > + return ret; > + } > + } > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > + s->snapshots_offset, s->snapshots_size); > + > + /* refcount data */ > + inc_refcounts(bs, res, *refcount_table, *nb_clusters, > + s->refcount_table_offset, > + s->refcount_table_size * sizeof(uint64_t)); > + > + return check_refblocks(bs, res, fix, refcount_table, nb_clusters); > +} > + > +/* > + * Checks an image for refcount consistency. > + * > + * Returns 0 if no errors are found, the number of errors in case the image is > + * detected as corrupted, and -errno when an internal error occurred. > + */ > +int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > + BdrvCheckMode fix) > +{ > + BDRVQcowState *s = bs->opaque; > + int64_t size, i, highest_cluster, nb_clusters; > + int refcount1, refcount2; > + uint16_t *refcount_table = NULL; > + int ret; > + > + size = bdrv_getlength(bs->file); > + if (size < 0) { > + res->check_errors++; > + return size; > + } > + > + nb_clusters = size_to_clusters(s, size); > + if (nb_clusters > INT_MAX) { > + res->check_errors++; > + return -EFBIG; > + } > + > + res->bfi.total_clusters = > + size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); > + > + ret = calculate_refcounts(bs, res, fix, &refcount_table, &nb_clusters); > + if (ret < 0) { > + goto fail; > + } > + > /* compare ref counts */ > for (i = 0, highest_cluster = 0; i < nb_clusters; i++) { > refcount1 = get_refcount(bs, i); > -- > 2.0.3 > Another point is that I think these two extractions patches will totaly confuse git blame the day we will need it. Best regards Benoît
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 <mreitz@redhat.com> >> --- >> 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.
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 <mreitz@redhat.com> > >> --- > >> 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 >
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 <mreitz@redhat.com> >>>> --- >>>> 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
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 <mreitz@redhat.com> > >>>>--- > >>>> 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
On 22.08.2014 17:37, Benoît Canet wrote: > 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 <mreitz@redhat.com> >>>>>> --- >>>>>> 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. This is not a consequence of the extracted functions moving up, but because of check_refblocks() preceding calculate_refcounts(). Splitting the check won't help; and I know that Eric dislikes (forward-)declarations of static functions (which I have to do if I pull calculate_refcounts() before check_refblocks()). Max
On 22.08.2014 17:44, Max Reitz wrote: > [snip] > > […] Splitting the check won't help; […] Sorry, I meant “patch” instead of “check”. Max
On 08/22/2014 09:44 AM, Max Reitz wrote: > This is not a consequence of the extracted functions moving up, but > because of check_refblocks() preceding calculate_refcounts(). Splitting > the check won't help; and I know that Eric dislikes > (forward-)declarations of static functions (which I have to do if I pull > calculate_refcounts() before check_refblocks()). I may not like them in the end result, but they are perfectly fine for an intermediate patch that is refactoring things, when coupled with a followup patch that moves the refactored code into topological order. Anything we can do to make reviewing easier, even if it requires going through more commits and more lines of churn to get there, pays off in the long run.
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index d60e2fe..b3c4edd 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1496,69 +1496,15 @@ done: } /* - * Checks an image for refcount consistency. - * - * Returns 0 if no errors are found, the number of errors in case the image is - * detected as corrupted, and -errno when an internal error occurred. + * Checks consistency of refblocks and accounts for each refblock in + * *refcount_table. */ -int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, - BdrvCheckMode fix) +static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res, + BdrvCheckMode fix, uint16_t **refcount_table, + int64_t *nb_clusters) { BDRVQcowState *s = bs->opaque; - int64_t size, i, highest_cluster, nb_clusters; - int refcount1, refcount2; - QCowSnapshot *sn; - uint16_t *refcount_table; - int ret; - - size = bdrv_getlength(bs->file); - if (size < 0) { - res->check_errors++; - return size; - } - - nb_clusters = size_to_clusters(s, size); - if (nb_clusters > INT_MAX) { - res->check_errors++; - return -EFBIG; - } - - refcount_table = g_try_malloc0(nb_clusters * sizeof(uint16_t)); - if (nb_clusters && refcount_table == NULL) { - res->check_errors++; - return -ENOMEM; - } - - res->bfi.total_clusters = - size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); - - /* header */ - inc_refcounts(bs, res, refcount_table, nb_clusters, - 0, s->cluster_size); - - /* current L1 table */ - ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters, - s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO); - if (ret < 0) { - goto fail; - } - - /* snapshots */ - for(i = 0; i < s->nb_snapshots; i++) { - sn = s->snapshots + i; - ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters, - sn->l1_table_offset, sn->l1_size, 0); - if (ret < 0) { - goto fail; - } - } - inc_refcounts(bs, res, refcount_table, nb_clusters, - s->snapshots_offset, s->snapshots_size); - - /* refcount data */ - inc_refcounts(bs, res, refcount_table, nb_clusters, - s->refcount_table_offset, - s->refcount_table_size * sizeof(uint64_t)); + int64_t i; for(i = 0; i < s->refcount_table_size; i++) { uint64_t offset, cluster; @@ -1573,7 +1519,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, continue; } - if (cluster >= nb_clusters) { + if (cluster >= *nb_clusters) { fprintf(stderr, "ERROR refcount block %" PRId64 " is outside image\n", i); res->corruptions++; @@ -1581,14 +1527,14 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, } if (offset != 0) { - inc_refcounts(bs, res, refcount_table, nb_clusters, + inc_refcounts(bs, res, *refcount_table, *nb_clusters, offset, s->cluster_size); - if (refcount_table[cluster] != 1) { + if ((*refcount_table)[cluster] != 1) { fprintf(stderr, "%s refcount block %" PRId64 " refcount=%d\n", fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", - i, refcount_table[cluster]); + i, (*refcount_table)[cluster]); if (fix & BDRV_FIX_ERRORS) { int64_t new_offset; @@ -1600,17 +1546,24 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, } /* update refcounts */ - if ((new_offset >> s->cluster_bits) >= nb_clusters) { + if ((new_offset >> s->cluster_bits) >= *nb_clusters) { /* increase refcount_table size if necessary */ - int old_nb_clusters = nb_clusters; - nb_clusters = (new_offset >> s->cluster_bits) + 1; - refcount_table = g_realloc(refcount_table, - nb_clusters * sizeof(uint16_t)); - memset(&refcount_table[old_nb_clusters], 0, (nb_clusters - - old_nb_clusters) * sizeof(uint16_t)); + int old_nb_clusters = *nb_clusters; + *nb_clusters = (new_offset >> s->cluster_bits) + 1; + + *refcount_table = g_try_realloc(*refcount_table, + *nb_clusters * sizeof(uint16_t)); + if (!*refcount_table) { + res->check_errors++; + return -ENOMEM; + } + + memset(&(*refcount_table)[old_nb_clusters], 0, + (*nb_clusters - old_nb_clusters) * + sizeof(uint16_t)); } - refcount_table[cluster]--; - inc_refcounts(bs, res, refcount_table, nb_clusters, + (*refcount_table)[cluster]--; + inc_refcounts(bs, res, *refcount_table, *nb_clusters, new_offset, s->cluster_size); res->corruptions_fixed++; @@ -1621,6 +1574,95 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, } } + return 0; +} + +/* + * Calculates an in-memory refcount table. + */ +static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res, + BdrvCheckMode fix, uint16_t **refcount_table, + int64_t *nb_clusters) +{ + BDRVQcowState *s = bs->opaque; + QCowSnapshot *sn; + int64_t i; + int ret; + + if (!*refcount_table) { + *refcount_table = g_try_malloc0(*nb_clusters * sizeof(uint16_t)); + if (*nb_clusters && !*refcount_table) { + res->check_errors++; + return -ENOMEM; + } + } + + /* header */ + inc_refcounts(bs, res, *refcount_table, *nb_clusters, + 0, s->cluster_size); + + /* current L1 table */ + ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters, + s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO); + if (ret < 0) { + return ret; + } + + /* snapshots */ + for (i = 0; i < s->nb_snapshots; i++) { + sn = s->snapshots + i; + ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters, + sn->l1_table_offset, sn->l1_size, 0); + if (ret < 0) { + return ret; + } + } + inc_refcounts(bs, res, *refcount_table, *nb_clusters, + s->snapshots_offset, s->snapshots_size); + + /* refcount data */ + inc_refcounts(bs, res, *refcount_table, *nb_clusters, + s->refcount_table_offset, + s->refcount_table_size * sizeof(uint64_t)); + + return check_refblocks(bs, res, fix, refcount_table, nb_clusters); +} + +/* + * Checks an image for refcount consistency. + * + * Returns 0 if no errors are found, the number of errors in case the image is + * detected as corrupted, and -errno when an internal error occurred. + */ +int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, + BdrvCheckMode fix) +{ + BDRVQcowState *s = bs->opaque; + int64_t size, i, highest_cluster, nb_clusters; + int refcount1, refcount2; + uint16_t *refcount_table = NULL; + int ret; + + size = bdrv_getlength(bs->file); + if (size < 0) { + res->check_errors++; + return size; + } + + nb_clusters = size_to_clusters(s, size); + if (nb_clusters > INT_MAX) { + res->check_errors++; + return -EFBIG; + } + + res->bfi.total_clusters = + size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); + + ret = calculate_refcounts(bs, res, fix, &refcount_table, &nb_clusters); + if (ret < 0) { + goto fail; + } + /* compare ref counts */ for (i = 0, highest_cluster = 0; i < nb_clusters; i++) { refcount1 = get_refcount(bs, i);
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(). Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2-refcount.c | 188 ++++++++++++++++++++++++++++++------------------- 1 file changed, 115 insertions(+), 73 deletions(-)