[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 02/10] qcow2: Split qcow2_check_refcounts()
From: |
Benoît Canet |
Subject: |
Re: [Qemu-devel] [PATCH v3 02/10] qcow2: Split qcow2_check_refcounts() |
Date: |
Fri, 22 Aug 2014 18:03:41 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Aug 22, 2014 at 06:31:36PM +0200, Max Reitz wrote:
> Put the code for calculating the reference counts and comparing them
> during qemu-img check into own functions.
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> block/qcow2-refcount.c | 153
> ++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 102 insertions(+), 51 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 43665b8..5f0920b 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1496,71 +1496,70 @@ done:
> return new_offset;
> }
>
> +static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
> + BdrvCheckMode fix, uint16_t **refcount_table,
> + int64_t *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.
> + * Calculates an in-memory refcount table.
> */
> -int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> - BdrvCheckMode fix)
> +static int calculate_refcounts(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;
> + int64_t i;
> 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_new0(uint16_t, nb_clusters);
> - if (nb_clusters && refcount_table == NULL) {
> + *refcount_table = g_try_new0(uint16_t, *nb_clusters);
> + 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,
> + 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,
> + 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;
> + return ret;
> }
>
> /* snapshots */
> - for(i = 0; i < s->nb_snapshots; i++) {
> + for (i = 0; i < s->nb_snapshots; i++) {
> sn = s->snapshots + i;
> - ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
> + ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters,
> sn->l1_table_offset, sn->l1_size, 0);
> if (ret < 0) {
> - goto fail;
> + return ret;
> }
> }
> - inc_refcounts(bs, res, refcount_table, nb_clusters,
> + inc_refcounts(bs, res, *refcount_table, *nb_clusters,
> s->snapshots_offset, s->snapshots_size);
>
> /* refcount data */
> - inc_refcounts(bs, res, refcount_table, nb_clusters,
> + 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 consistency of refblocks and accounts for each refblock in
> + * *refcount_table.
> + */
> +static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
> + BdrvCheckMode fix, uint16_t **refcount_table,
> + int64_t *nb_clusters)
> +{
> + BDRVQcowState *s = bs->opaque;
> + int64_t i;
> +
> for(i = 0; i < s->refcount_table_size; i++) {
> uint64_t offset, cluster;
> offset = s->refcount_table[i];
> @@ -1574,7 +1573,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++;
> @@ -1582,14 +1581,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;
> @@ -1601,17 +1600,18 @@ 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_renew(uint16_t, refcount_table,
> - nb_clusters);
> - 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_renew(uint16_t, *refcount_table,
> + *nb_clusters);
> + 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++;
> @@ -1622,8 +1622,22 @@ int qcow2_check_refcounts(BlockDriverState *bs,
> BdrvCheckResult *res,
> }
> }
>
> - /* compare ref counts */
> - for (i = 0, highest_cluster = 0; i < nb_clusters; i++) {
> + return 0;
> +}
> +
> +/*
> + * Compares the actual reference count for each cluster in the image against
> the
> + * refcount as reported by the refcount structures on-disk.
> + */
> +static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> + BdrvCheckMode fix, int64_t *highest_cluster,
> + uint16_t *refcount_table, int64_t nb_clusters)
> +{
> + BDRVQcowState *s = bs->opaque;
> + int64_t i;
> + int refcount1, refcount2, ret;
> +
> + for (i = 0, *highest_cluster = 0; i < nb_clusters; i++) {
> refcount1 = get_refcount(bs, i);
> if (refcount1 < 0) {
> fprintf(stderr, "Can't get refcount for cluster %" PRId64 ":
> %s\n",
> @@ -1635,11 +1649,10 @@ int qcow2_check_refcounts(BlockDriverState *bs,
> BdrvCheckResult *res,
> refcount2 = refcount_table[i];
>
> if (refcount1 > 0 || refcount2 > 0) {
> - highest_cluster = i;
> + *highest_cluster = i;
> }
>
> if (refcount1 != refcount2) {
> -
> /* Check if we're allowed to fix the mismatch */
> int *num_fixed = NULL;
> if (refcount1 > refcount2 && (fix & BDRV_FIX_LEAKS)) {
> @@ -1672,6 +1685,44 @@ int qcow2_check_refcounts(BlockDriverState *bs,
> BdrvCheckResult *res,
> }
> }
> }
> +}
> +
> +/*
> + * 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, highest_cluster, nb_clusters;
> + 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;
> + }
> +
> + 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_refcounts(bs, res, fix, &highest_cluster, refcount_table,
> + nb_clusters);
>
> /* check OFLAG_COPIED */
> ret = check_oflag_copied(bs, res, fix);
> --
> 2.0.4
>
Thanks it's nicer to read.
My opinion changed from "It must be good Max took care of it" to "I did not
find any bug" :)
Reviewed-by: Benoît Canet <address@hidden>
- [Qemu-devel] [PATCH v3 00/10] qcow2: Fix image repairing, Max Reitz, 2014/08/22
- [Qemu-devel] [PATCH v3 01/10] qcow2: Fix leaks in dirty images, Max Reitz, 2014/08/22
- [Qemu-devel] [PATCH v3 02/10] qcow2: Split qcow2_check_refcounts(), Max Reitz, 2014/08/22
- Re: [Qemu-devel] [PATCH v3 02/10] qcow2: Split qcow2_check_refcounts(),
Benoît Canet <=
- [Qemu-devel] [PATCH v3 03/10] qcow2: Pull check_refblocks() up, Max Reitz, 2014/08/22
- [Qemu-devel] [PATCH v3 04/10] qcow2: Reuse refcount table in calculate_refcounts(), Max Reitz, 2014/08/22
- [Qemu-devel] [PATCH v3 05/10] qcow2: Fix refcount blocks beyond image end, Max Reitz, 2014/08/22
- [Qemu-devel] [PATCH v3 06/10] qcow2: Do not perform potentially damaging repairs, Max Reitz, 2014/08/22