Re: [Qemu-block] [PATCH 21/24] qcow2-bitmap: refcounts

[Max Reitz]

On 23.01.2017 13:10, Vladimir Sementsov-Ogievskiy wrote:
> Calculate refcounts for qcow2 bitmaps. It is needed for qcow2's qemu-img
> check implementation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/qcow2-bitmap.c   | 80 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2-refcount.c |  6 ++++
>  block/qcow2.h          |  3 ++
>  3 files changed, 89 insertions(+)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 8b8f2d11ec..2687a3acd5 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1232,3 +1232,83 @@ common_errp:
>                 name, bdrv_get_device_or_node_name(bs), reason);
>      return false;
>  }
> +
> +int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> +                                  void **refcount_table,
> +                                  int64_t *refcount_table_size)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    Qcow2BitmapList *bm_list;
> +    Qcow2Bitmap *bm;
> +
> +    if (s->nb_bitmaps == 0) {
> +        return 0;
> +    }
> +
> +    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
> refcount_table_size,
> +                                   s->bitmap_directory_offset,
> +                                   s->bitmap_directory_size);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +                               s->bitmap_directory_size, NULL);
> +    if (bm_list == NULL) {
> +        res->corruptions++;
> +        return -EINVAL;
> +    }
> +
> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +        uint64_t *bitmap_table = NULL;
> +        int i;
> +
> +        ret = qcow2_inc_refcounts_imrt(bs, res,
> +                                       refcount_table, refcount_table_size,
> +                                       bm->table.offset,
> +                                       bm->table.size * sizeof(uint64_t));
> +        if (ret < 0) {
> +            goto out;
> +        }
> +
> +        ret = bitmap_table_load(bs, &bm->table, &bitmap_table);
> +        if (ret < 0) {
> +            res->corruptions++;
> +            goto out;
> +        }
> +
> +        for (i = 0; i < bm->table.size; ++i) {
> +            uint64_t entry = bitmap_table[i];
> +            uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
> +
> +            if (check_table_entry(entry, s->cluster_size) < 0) {
> +                res->corruptions++;
> +                continue;
> +            }
> +
> +            if (offset == 0) {
> +                continue;
> +            }
> +
> +            ret = qcow2_inc_refcounts_imrt(bs, res,
> +                                           refcount_table, 
> refcount_table_size,
> +                                           offset, s->cluster_size);
> +            if (ret < 0) {
> +                g_free(bitmap_table);
> +                goto out;
> +            }
> +        }
> +
> +        g_free(bitmap_table);
> +    }
> +
> +    if (res->corruptions > 0) {
> +        ret = -EINVAL;
> +    }

res->corruptions may already be greater than zero when this function is
invoked.

But in any case, you don't need to return a negative value just because
some corruption was found. These check functions should only return
negative values if they were unable to do what they are supposed to do
(whatever that is), e.g. because of an I/O error or ENOMEM. In these
cases, however, they usually return to the caller immediately.

It wouldn't be strictly wrong to return a negative value if some table
entry is corrupted, but I don't think it's necessary either. So if you
just drop the above three lines:

Reviewed-by: Max Reitz <address@hidden>

signature.asc
Description: OpenPGP digital signature