19.12.2016 18:14, Max Reitz wrote:
On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote:
09.12.2016 20:05, Max Reitz wrote:
On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote:
Realize block bitmap storing interface, to allow qcow2 images store
persistent bitmaps.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++
block/qcow2.c | 1 +
[...]
+
+/* store_bitmap_data()
+ * Store bitmap to image, filling bitmap table accordingly.
+ */
+static uint64_t *store_bitmap_data(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ uint32_t *bitmap_table_size, Error **errp)
+{
+ int ret;
+ BDRVQcow2State *s = bs->opaque;
+ int64_t sector;
+ uint64_t dsc;
+ uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
+ const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
+ uint8_t *buf = NULL;
+ BdrvDirtyBitmapIter *dbi;
+ uint64_t *tb;
+ uint64_t tb_size =
+ size_to_clusters(s,
+ bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));
+
+ if (tb_size > BME_MAX_TABLE_SIZE ||
+ tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) {
Alignment to the opening parenthesis, please.
+ error_setg(errp, "Bitmap '%s' is too big", bm_name);
+ return NULL;
+ }
+
+ tb = g_try_new0(uint64_t, tb_size);
+ if (tb == NULL) {
+ error_setg(errp, "No memory");
+ return NULL;
+ }
+
+ dbi = bdrv_dirty_iter_new(bitmap, 0);
+ buf = g_malloc(s->cluster_size);
+ dsc = disk_sectors_in_bitmap_cluster(s, bitmap);
+
+ while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
+ uint64_t cluster = sector / dsc;
+ uint64_t end, write_size;
+ int64_t off;
+
+ sector = cluster * dsc;
+ end = MIN(bm_size, sector + dsc);
+ write_size =
+ bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);
+
+ off = qcow2_alloc_clusters(bs, s->cluster_size);
+ if (off < 0) {
+ error_setg_errno(errp, -off,
+ "Failed to allocate clusters for bitmap '%s'",
+ bm_name);
+ goto fail;
+ }
+ tb[cluster] = off;
Somehow I would feel better with either an assert(cluster < tb_size);
here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error
handling for bdrv_nb_sectors()) above the loop.
assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional
error handling. Right?
Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not
necessarily a multiple of dsc. So that should be good. Alternatively, I
think the following would be slightly easier to read:
assert(DIV_ROUND_UP(bm_size, dsc) == tb_size);
+
+ bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector);
+ if (write_size < s->cluster_size) {
+ memset(buf + write_size, 0, s->cluster_size - write_size);
+ }
Should we assert that write_size <= s->cluster_size?
Ok
[...].
+ const char *name = bdrv_dirty_bitmap_name(bitmap);
+ uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+ Qcow2Bitmap *bm;
+
+ if (!bdrv_dirty_bitmap_get_persistance(bitmap)) {
+ continue;
+ }
+
+ if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) {
+ error_setg(errp, "Too many persistent bitmaps");
+ goto fail;
+ }
+
+ new_dir_size += calc_dir_entry_size(strlen(name), 0);
+ if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
+ error_setg(errp, "Too large bitmap directory");
+ goto fail;
+ }
You only need to increment new_nb_bitmaps and increase new_dir_size if
the bitmap does not already exist in the image (i.e. if
find_bitmap_by_name() below returns NULL).
Why? No, I need to check the whole sum and the whole size.
If the bitmap already exists, you don't create a new directory entry but
reuse the existing one. Therefore, the number of bitmaps in the image
and the directory size will not grow then.
new_nb_bitmaps is not number of "newly created bitmaps", but just new
value of field nb_bitmaps, so, all bitmaps - old and new are calculated
into new_nb_bitmaps. Anyway, this misunderstanding shows that variable
name is bad..
Max
+
+ if (check_constraints_on_bitmap(bs, name, granularity) < 0) {
+ error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints",
+ name);
+ goto fail;
+ }
+
+ bm = find_bitmap_by_name(bm_list, name);
+ if (bm == NULL) {
+ bm = g_new0(Qcow2Bitmap, 1);
+ bm->name = g_strdup(name);
+ QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
+ } else {
+ if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) {
Shouldn't we error out right at the beginning of this function if
can_write(bs) is false?
Hmm, right.
[...]
--
Best regards,
Vladimir
--
Best regards,
Vladimir
