On 19.12.2016 16:26, Vladimir Sementsov-Ogievskiy wrote: > 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 <[email protected]> >>>>> --- >>>>> 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..
Yes. But when you store a bitmap of the same name as an existing one, you are replacing it. The number of bitmaps does not grow in that case. Max
signature.asc
Description: OpenPGP digital signature
