On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > Calculate refcounts for dirty bitmaps.
The commit message should mention that this is for qcow2's qemu-img
check implementation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> block/qcow2-bitmap.c | 60
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> block/qcow2-refcount.c | 14 +++++++-----
> block/qcow2.h | 5 +++++
> 3 files changed, 74 insertions(+), 5 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 76f7e2b..6d9394a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -901,3 +901,63 @@ out:
> g_free(new_dir);
> g_free(dir);
> }
> +
> +int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs,
> + BdrvCheckResult *res,
> + void **refcount_table,
> + int64_t *refcount_table_size)
I'd rename this function to make clear that this is for checking the
refcounts, e.g. to "qcow2_check_dirty_bitmaps_refcounts" or
"qcow2_count_dirty_bitmaps_refcounts" or just
"qcow2_check_dirty_bitmaps". Probably the last one is the best because
this function should ideally perform a full check of the dirty bitmaps
extension.
> +{
> + BDRVQcow2State *s = bs->opaque;
> + uint8_t *dir;
> + Qcow2BitmapDirEntry *e;
> + Error *local_err = NULL;
> +
> + int i;
> + int ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> + s->bitmap_directory_offset,
> + s->bitmap_directory_size);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + dir = directory_read(bs, s->bitmap_directory_offset,
> + s->bitmap_directory_size, &local_err);
> + if (dir == NULL) {
> + error_report_err(local_err);
I think you should increment res->corruptions here.
> + return -EINVAL;
> + }
> +
> + for_each_bitmap_dir_entry(e, dir, s->bitmap_directory_size) {
> + uint64_t *bitmap_table = NULL;
I think you should call check_constraints() here.
> +
> + ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> + e->bitmap_table_offset,
> + e->bitmap_table_size);
Probably rather e->bitmap_table_size * sizeof(uint64_t).
> + if (ret < 0) {
> + return ret;
> + }
> +
> + ret = bitmap_table_load(bs, e, &bitmap_table);
> + if (ret < 0) {
Again, it would make sense to increment res->corruptions here.
> + return ret;
> + }
> +
> + for (i = 0; i < e->bitmap_table_size; ++i) {
> + uint64_t off = be64_to_cpu(bitmap_table[i]);
> + if (off <= 1) {
> + continue;
> + }
As I said in some other patch, I'd write this condition differently
(with an offset mask, etc.).
Also, you should check that the offset is aligned to a cluster boundary
and that no unknown flags are set.
> +
> + ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> + off, s->cluster_size);
> + if (ret < 0) {
> + g_free(bitmap_table);
> + return ret;
> + }
> + }
> +
> + g_free(bitmap_table);
> + }
> +
> + return 0;
dir is leaked here.
> +}
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index cbfb3fe..05bcc57 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1309,11 +1309,9 @@ static int realloc_refcount_array(BDRVQcow2State *s,
> void **array,
> *
> * Modifies the number of errors in res.
> */
> -static int inc_refcounts(BlockDriverState *bs,
> - BdrvCheckResult *res,
> - void **refcount_table,
> - int64_t *refcount_table_size,
> - int64_t offset, int64_t size)
> +int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> + void **refcount_table, int64_t *refcount_table_size,
> + int64_t offset, int64_t size)
First, if you make this function public, you have to give it a qcow2_
prefix.
Second, if this function is public, it should have a name that makes
sense. inc_refcounts() sounds as if it's the same as update_refcount()
with an addend of 1. I'd rename it qcow2_inc_refcounts_imrt(), because
that's probably the shortest name I can come up with (and
qcow2-refcount.c explains what an IMRT is in some comment).
> {
> BDRVQcow2State *s = bs->opaque;
> uint64_t start, last, cluster_offset, k, refcount;
> @@ -1843,6 +1841,12 @@ static int calculate_refcounts(BlockDriverState *bs,
> BdrvCheckResult *res,
> return ret;
> }
>
> + /* dirty bitmaps */
> + ret = qcow2_dirty_bitmaps_refcounts(bs, res, refcount_table,
> nb_clusters);
> + if (ret < 0) {
> + return ret;
> + }
> +
> return check_refblocks(bs, res, fix, rebuild, refcount_table,
> nb_clusters);
> }
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index a5e7592..0a050ea 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -563,6 +563,9 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs,
> int ign, int64_t offset,
> int64_t size);
> int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t
> offset,
> int64_t size);
> +int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> + void **refcount_table, int64_t *refcount_table_size,
> + int64_t offset, int64_t size);
>
> int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
> BlockDriverAmendStatusCB *status_cb,
> @@ -616,6 +619,8 @@ int qcow2_read_snapshots(BlockDriverState *bs);
> int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
>
> int qcow2_delete_bitmaps(BlockDriverState *bs);
> +int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> + void **refcount_table, int64_t *refcount_table_size);
Normally we try to align parameters along the opening parenthesis as
long as there is enough space, and there is enough space here.
Max
>
> /* qcow2-cache.c functions */
> Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
>
signature.asc
Description: OpenPGP digital signature
