On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are
*with the AUTO flag set > loaded at image open and becomes BdrvDirtyBitmap's for corresponding "loaded when the image is opened and become BdrvDirtyBitmaps for the corresponding drive." > drive. These bitmaps are deleted from Qcow2 image after loading to avoid *from the Qcow2 image > conflicts. > > Extra data in bitmaps is not supported for now. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > block/qcow2-bitmap.c | 518 > ++++++++++++++++++++++++++++++++++++++++++++++++++- > block/qcow2.c | 5 + > block/qcow2.h | 3 + > 3 files changed, 525 insertions(+), 1 deletion(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index cd18b07..760a047 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -25,6 +25,12 @@ [...] > +static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table, > + uint32_t bitmap_table_size) > +{ > + BDRVQcow2State *s = bs->opaque; > + int cl_size = s->cluster_size; > + int i; > + > + for (i = 0; i < bitmap_table_size; ++i) { > + uint64_t addr = bitmap_table[i]; > + if (addr <= 1) { I'd rather add a BME_TABLE_ENTRY_OFFSET_MASK (0x00fffffffffffe00ull) and then use: uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK; if (!addr) { > + continue; > + } > + > + qcow2_free_clusters(bs, addr, cl_size, QCOW2_DISCARD_ALWAYS); I think this should rather be QCOW2_DISCARD_OTHER; or you introduce a new QCOW2_DISCARD_* flag. (The same applies to all the other QCOW2_DISCARD_ALWAYS users here.) Also, I don't really see the point of introducing cl_size just for this place; it doesn't even save you from a line wrap. > + bitmap_table[i] = 0; > + } > +} > + > +static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapDirEntry > *entry, > + uint64_t **table) > +{ > + int ret; > + > + *table = g_try_new(uint64_t, entry->bitmap_table_size); > + if (*table == NULL) { Not that g_try_new() will return NULL if you try to allocate a buffer with size 0. Normally, entry->bitmap_table_size shouldn't be 0, but I don't think you're catching that case anywhere. > + return -ENOMEM; > + } > + I wouldn't mind an assert(entry->bitmap_table_size <= UINT32_MAX / sizeof(uint64_t)); here because of the following multiplication (which is extended to 64 bit on 64 bit machines, but stays in 32 bit on 32 bit machines). (Of course, assert(entry->bitmap_table_size <= BME_MAX_TABLE_SIZE); would be fine, too.) > + ret = bdrv_pread(bs->file, entry->bitmap_table_offset, > + *table, entry->bitmap_table_size * sizeof(uint64_t)); > + if (ret < 0) { > + g_free(*table); > + *table = NULL; > + return ret; > + } > + > + bitmap_table_to_cpu(*table, entry->bitmap_table_size); > + > + return 0; > +} > + > +static void do_free_bitmap_clusters(BlockDriverState *bs, > + uint64_t table_offset, > + uint32_t table_size, > + uint64_t *bitmap_table) > +{ > + clear_bitmap_table(bs, bitmap_table, table_size); > + > + qcow2_free_clusters(bs, table_offset, table_size * sizeof(uint64_t), > + QCOW2_DISCARD_ALWAYS); > +} > + > +static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapDirEntry > *entry) > +{ > + int ret; > + uint64_t *bitmap_table; > + > + ret = bitmap_table_load(bs, entry, &bitmap_table); > + if (ret < 0 || bitmap_table == NULL) { That should be: if (ret < 0) { assert(bitmap_table == NULL); Or the other way around: if (bitmap_table == NULL) { assert(ret < 0); Otherwise, it looks like bitmap_table may be NULL with ret being 0, and the next statement would make this function return success even though it actually failed. > + return ret; > + } > + > + do_free_bitmap_clusters(bs, entry->bitmap_table_offset, > + entry->bitmap_table_size, bitmap_table); > + > + return 0; You're leaking bitmap_table here. > +} > + > +/* dirty sectors in cluster is a number of sectors in the image, > corresponding > + * to one cluster of bitmap data */ How about: This function returns the number of sectors covered by a single cluster of dirty bitmap data. > +static uint64_t dirty_sectors_in_cluster(const BDRVQcow2State *s, > + const BdrvDirtyBitmap *bitmap) > +{ > + uint32_t sector_granularity = > + bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS; > + > + return (uint64_t)sector_granularity * (s->cluster_size << 3); > +} > + > +static int load_bitmap_data(BlockDriverState *bs, > + const uint64_t *dirty_bitmap_table, > + uint32_t dirty_bitmap_table_size, > + BdrvDirtyBitmap *bitmap) > +{ > + int ret = 0; > + BDRVQcow2State *s = bs->opaque; > + uint64_t sector, dsc; > + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); > + int cl_size = s->cluster_size; Once more, I don't really see the reason to put this into a dedicated variable (I feel like it makes the code harder to read because you have to keep in mind what cl_size is). > + uint8_t *buf = NULL; > + uint32_t i, tab_size = > + size_to_clusters(s, > + bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size)); > + > + if (tab_size != dirty_bitmap_table_size) { > + return -EINVAL; > + } You should also check that tab_size does not exceed BME_MAX_TABLE_SIZE. And maybe you should check that the physical size of the bitmap data does not exceed BME_MAX_PHYS_SIZE, but I'm not so sure why you have that constant at all because it's basically superseded by the existence of BME_MAX_TABLE_SIZE. > + > + bdrv_clear_dirty_bitmap(bitmap, NULL); > + > + buf = g_malloc0(cl_size); g_malloc() would suffice as well, since you'll be overwriting all of it anyway. > + dsc = dirty_sectors_in_cluster(s, bitmap); > + for (i = 0, sector = 0; i < tab_size; ++i, sector += dsc) { > + uint64_t end = MIN(bm_size, sector + dsc); > + uint64_t offset = dirty_bitmap_table[i]; > + > + if (offset == 1) { > + bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, end, false); > + } else if (offset > 1) { As before, I'd organize this differently, e.g. like: uint64_t table_entry = dirty_bitmap_table[i]; uint64_t offset = table_entry & BME_TABLE_ENTRY_OFFSET_MASK; if (table_entry & BME_TABLE_ENTRY_RESERVED_MASK) { ret = -EINVAL; goto finish; } if (!offset) { if (table_entry & 1) { bdrv_dirty_bitmap_deserialize_ones(bitmap, sector, end, false); } else { /* No need to deserialize zeroes because the dirty bitmap is * already cleared */ } } else { ret = bdrv_pread(...); ... } It's more code, yes, but I find it easier to read because it reflects the specification better. > + ret = bdrv_pread(bs->file, offset, buf, cl_size); > + if (ret < 0) { > + goto finish; > + } > + bdrv_dirty_bitmap_deserialize_part(bitmap, buf, sector, end, > false); > + } > + } > + ret = 0; > + > + bdrv_dirty_bitmap_deserialize_finish(bitmap); > + > +finish: > + g_free(buf); > + > + return ret; > +} > + > +static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs, > + Qcow2BitmapDirEntry *entry, Error **errp) > +{ > + int ret; > + uint64_t *bitmap_table = NULL; > + uint32_t granularity; > + BdrvDirtyBitmap *bitmap = NULL; > + char *name = g_strndup(dir_entry_name_notcstr(entry), entry->name_size); Maybe you should limit entry->name_size to BME_MAX_NAME_SIZE. > + > + ret = bitmap_table_load(bs, entry, &bitmap_table); > + if (ret < 0) { > + error_setg_errno(errp, -ret, > + "Could not read bitmap_table table from image"); > + goto fail; > + } > + > + granularity = 1U << entry->granularity_bits; You should be making sure that entry->granularity_bits does not exceed [BME_MIN_GRANULARITY_BITS, BME_MAX_GRANULARITY_BITS] before this point. > + bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp); > + if (bitmap == NULL) { > + goto fail; > + } > + > + ret = load_bitmap_data(bs, bitmap_table, entry->bitmap_table_size, > + bitmap); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Could not read bitmap from image"); > + goto fail; > + } > + > + g_free(name); > + g_free(bitmap_table); > + return bitmap; > + > +fail: > + g_free(name); > + g_free(bitmap_table); > + if (bitmap != NULL) { > + bdrv_release_dirty_bitmap(bs, bitmap); > + } > + > + return NULL; > +} > + > +/* directory_read > + * Read bitmaps directory from bs by @offset and @size. Convert it to cpu > + * format from BE. > + */ > +static uint8_t *directory_read(BlockDriverState *bs, > + uint64_t offset, uint64_t size, Error **errp) > +{ > + int ret; > + uint8_t *dir, *dir_end; > + Qcow2BitmapDirEntry *e; I think you should return an error if size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE. > + > + dir = g_try_malloc(size); > + if (dir == NULL) { > + error_setg(errp, "Can't allocate space for bitmap directory."); Please drop the full stop at the end of the message. (And I personally don't like contractions ("can't") in user-facing messages too much, but that is just my taste.) > + return NULL; > + } > + dir_end = dir + size; > + > + ret = bdrv_pread(bs->file, offset, dir, size); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Can't read bitmap directory."); Again, no full stop at the end of error messages, please. > + goto fail; > + } > + > + /* loop is safe because next entry offset is calculated after conversion > to Should it be s/safe/unsafe/? > + * cpu format */ > + for_each_bitmap_dir_entry_unsafe(e, dir, size) { > + if ((uint8_t *)(e + 1) > dir_end) { > + goto broken_dir; > + } > + > + bitmap_dir_entry_to_cpu(e); > + > + if ((uint8_t *)next_dir_entry(e) > dir_end) { > + goto broken_dir; > + } > + > + if (e->extra_data_size != 0) { > + error_setg(errp, "Can't load bitmap '%.*s' from '%s':" > + "extra data not supported.", e->name_size, Full stop again. Also, I'm not quite sure why you give the device/node name here. You don't do that anywhere else and I think if we want to emit the information where something failed, it should be added at some previous point in the call chain. > + dir_entry_name_notcstr(e), > + bdrv_get_device_or_node_name(bs)); > + goto fail; > + } > + } Maybe you should count the number of dirty bitmaps and return an error if it exceeds QCOW_MAX_DIRTY_BITMAPS. > + > + assert((uint8_t *)e == dir_end); I think this should rather go to broken_dir if they're not equal. > + > + return dir; > + > +broken_dir: > + error_setg(errp, "Broken bitmap directory."); And again. > + > +fail: > + g_free(dir); > + > + return NULL; > +} > + > +static int update_header_sync(BlockDriverState *bs) > +{ > + int ret; > + > + ret = qcow2_update_header(bs); > + if (ret < 0) { > + return ret; > + } > + > + ret = bdrv_flush(bs); > + if (ret < 0) { > + return ret; > + } > + > + return 0; > +} > + > +/* write bitmap directory from the state to new allocated clusters */ > +static int64_t directory_write(BlockDriverState *bs, const uint8_t *dir, > + size_t size) > +{ > + int ret = 0; > + uint8_t *dir_be = NULL; > + int64_t dir_offset = 0; > + > + dir_be = g_try_malloc(size); > + if (dir_be == NULL) { > + return -ENOMEM; > + } > + memcpy(dir_be, dir, size); > + bitmap_directory_to_be(dir_be, size); > + > + /* Allocate space for the new bitmap directory */ > + dir_offset = qcow2_alloc_clusters(bs, size); > + if (dir_offset < 0) { > + ret = dir_offset; > + goto out; > + } > + > + /* The bitmap directory position has not yet been updated, so these > + * clusters must indeed be completely free */ > + ret = qcow2_pre_write_overlap_check(bs, 0, dir_offset, size); > + if (ret < 0) { > + goto out; > + } > + > + ret = bdrv_pwrite(bs->file, dir_offset, dir_be, size); > + if (ret < 0) { > + goto out; > + } > + > +out: > + g_free(dir_be); > + > + if (ret < 0) { > + if (dir_offset > 0) { > + qcow2_free_clusters(bs, dir_offset, size, QCOW2_DISCARD_ALWAYS); > + } > + > + return ret; > + } > + > + return dir_offset; > +} > + > +static int directory_update(BlockDriverState *bs, uint8_t *new_dir, > + size_t new_size, uint32_t new_nb_bitmaps) > +{ > + BDRVQcow2State *s = bs->opaque; > + int ret; > + int64_t new_offset = 0; > + uint64_t old_offset = s->bitmap_directory_offset; > + uint64_t old_size = s->bitmap_directory_size; > + uint32_t old_nb_bitmaps = s->nb_bitmaps; > + uint64_t old_autocl = s->autoclear_features; > + > + if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) { > + return -EINVAL; > + } Also, you should probably check new_nb_bitmaps against QCOW_MAX_DIRTY_BITMAPS, but maybe you'll be adding that check somewhere else in a future patch. In that case, an assertion wouldn't be too bad here, though. > + > + if ((new_size == 0) != (new_nb_bitmaps == 0)) { > + return -EINVAL; > + } > + > + if (new_nb_bitmaps > 0) { > + new_offset = directory_write(bs, new_dir, new_size); > + if (new_offset < 0) { > + return new_offset; > + } > + > + ret = bdrv_flush(bs); > + if (ret < 0) { > + goto fail; > + } > + } > + s->bitmap_directory_offset = new_offset; > + s->bitmap_directory_size = new_size; > + s->nb_bitmaps = new_nb_bitmaps; > + > + ret = update_header_sync(bs); > + if (ret < 0) { > + goto fail; > + } > + > + if (old_size) { > + qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS); > + } > + > + return 0; > + > +fail: > + if (new_offset > 0) { > + qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS); > + s->bitmap_directory_offset = old_offset; > + s->bitmap_directory_size = old_size; > + s->nb_bitmaps = old_nb_bitmaps; > + s->autoclear_features = old_autocl; Why are you restoring the autoclear features? From a quick glance I can't see any code path that changes this field here, and if there is one, it probably has a good reason to do so. > + } > + > + return ret; > +} > + > +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp) > +{ > + int ret; > + BDRVQcow2State *s = bs->opaque; > + uint8_t *dir, *new_dir, *new_pos; > + uint64_t dir_size; I suggest initializing this variable here (to s->bitmap_directory_size)... > + Qcow2BitmapDirEntry *e; > + uint32_t new_nb_bitmaps = 0; > + > + if (s->nb_bitmaps == 0) { > + /* No bitmaps - nothing to do */ > + return 0; > + } > + > + new_pos = new_dir = g_try_malloc(s->bitmap_directory_size); ...and then using it here... > + if (new_dir == NULL) { > + error_setg(errp, "Can't allocate space for bitmap directory."); > + return -ENOMEM; > + } > + > + dir_size = s->bitmap_directory_size; > + dir = directory_read(bs, s->bitmap_directory_offset, > + s->bitmap_directory_size, errp); ...and here. Alternatively, of course, you can just drop the variable and use s->bitmap_directory_size everywhere. > + if (dir == NULL) { > + ret = -EINVAL; > + goto out; > + } > + > + for_each_bitmap_dir_entry(e, dir, dir_size) { > + uint32_t fl = e->flags; > + > + if (fl & BME_FLAG_AUTO) { > + BdrvDirtyBitmap *bitmap = load_bitmap(bs, e, errp); > + if (bitmap == NULL) { > + ret = -EINVAL; > + goto out; > + } > + } else { > + /* leave bitmap in the image */ > + new_pos += copy_dir_entry(new_pos, e); > + new_nb_bitmaps++; > + } > + } > + > + if (!can_write(bs)) { You should be setting ret here (gcc complains about that). > + goto out; > + } > + > + ret = directory_update(bs, new_dir, new_pos - new_dir, new_nb_bitmaps); > + if (ret < 0) { > + error_setg(errp, "Can't update bitmap directory."); And once more, please no full stops at the end of error messages. > + goto out; > + } > + > + /* to be consistent, free bitmap only after successfull directory update > */ *successful > + for_each_bitmap_dir_entry(e, dir, dir_size) { > + uint32_t fl = e->flags; > + > + if (fl & BME_FLAG_AUTO) { > + free_bitmap_clusters(bs, e); > + } > + } > + > +out: > + g_free(dir); > + g_free(new_dir); > + > + return ret; > +} > diff --git a/block/qcow2.c b/block/qcow2.c > index 08c4ef9..02ec224 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -213,6 +213,11 @@ static int qcow2_read_extensions(BlockDriverState *bs, > uint64_t start_offset, > s->bitmap_directory_size = > bitmaps_ext.bitmap_directory_size; > > + ret = qcow2_read_bitmaps(bs, errp); > + if (ret < 0) { > + return ret; > + } > + I think I'd put this directly into qcow2_open(), just like qcow2_read_snapshots(); but that's an optional suggestion. Max > #ifdef DEBUG_EXT > printf("Qcow2: Got dirty bitmaps extension:" > " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n", > diff --git a/block/qcow2.h b/block/qcow2.h > index c068b2c..482a29f 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -603,6 +603,9 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, > void qcow2_free_snapshots(BlockDriverState *bs); > int qcow2_read_snapshots(BlockDriverState *bs); > > +/* qcow2-bitmap.c functions */ > +int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp); > + > /* qcow2-cache.c functions */ > Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables); > int qcow2_cache_destroy(BlockDriverState* bs, Qcow2Cache *c); >
signature.asc
Description: OpenPGP digital signature