Am 12.09.2012 09:28, schrieb Dong Xu Wang:
>>> +static bool is_allocated(BlockDriverState *bs, int64_t sector_num)
>>> +{
>>> + BDRVAddCowState *s = bs->opaque;
>>> + BlockCache *c = s->bitmap_cache;
>>> + int64_t cluster_num = sector_num / SECTORS_PER_CLUSTER;
>>> + uint8_t *table = NULL;
>>> + uint64_t offset = ADD_COW_PAGE_SIZE * s->header.header_pages_size
>>> + + (offset_in_bitmap(sector_num) & (~(c->entry_size - 1)));
>>> + int ret = block_cache_get(bs, s->bitmap_cache, offset,
>>> + (void **)&table, BLOCK_TABLE_BITMAP, ADD_COW_CACHE_ENTRY_SIZE);
>>
>> No matching block_cache_put?
>>
>>> +
>>> + if (ret < 0) {
>>> + return ret;
>>> + }
>>> + return table[cluster_num / 8 % ADD_COW_CACHE_ENTRY_SIZE]
>>> + & (1 << (cluster_num % 8));
>>> +}
>>> +
>>> +static coroutine_fn int add_cow_is_allocated(BlockDriverState *bs,
>>> + int64_t sector_num, int nb_sectors, int *num_same)
>>> +{
>>> + BDRVAddCowState *s = bs->opaque;
>>> + int changed;
>>> +
>>> + if (nb_sectors == 0) {
>>> + *num_same = 0;
>>> + return 0;
>>> + }
>>> +
>>> + if (s->header.features & ADD_COW_F_All_ALLOCATED) {
>>> + *num_same = nb_sectors - 1;
>>
>> Why - 1?
>>
>>> + return 1;
>>> + }
>>> + changed = is_allocated(bs, sector_num);
>>> +
>>> + for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
>>> + if (is_allocated(bs, sector_num + *num_same) != changed) {
>>> + break;
>>> + }
>>> + }
>>> + return changed;
>>> +}
>>> +static int bdrv_add_cow_truncate(BlockDriverState *bs, int64_t size)
>>> +{
>>> + BDRVAddCowState *s = bs->opaque;
>>> + int sector_per_byte = SECTORS_PER_CLUSTER * 8;
>>> + int ret;
>>> + uint32_t bitmap_pos = s->header.header_pages_size * ADD_COW_PAGE_SIZE;
>>> + int64_t bitmap_size =
>>> + (size / BDRV_SECTOR_SIZE + sector_per_byte - 1) / sector_per_byte;
>>> + bitmap_size = (bitmap_size + ADD_COW_CACHE_ENTRY_SIZE - 1)
>>> + & (~(ADD_COW_CACHE_ENTRY_SIZE - 1));
>>> +
>>> + ret = bdrv_truncate(bs->file, bitmap_pos + bitmap_size);
>>> + if (ret < 0) {
>>> + return ret;
>>> + }
>>> + return 0;
>>> +}
>>
>> So you don't truncate s->image_file? Does this work?
>
> s->image_file should be truncated? Image file can have a larger virtual size
> than backing_file, my understanding is we should not truncate image file.
I'm talking about s->image_hd, not bs->backing_hd. You are right that
the backing file should not be changed. But the associated raw image
should be resized, shouldn't it?
>>> +static coroutine_fn int add_cow_co_flush(BlockDriverState *bs)
>>> +{
>>> + BDRVAddCowState *s = bs->opaque;
>>> + int ret;
>>> +
>>> + qemu_co_mutex_lock(&s->lock);
>>> + ret = block_cache_flush(bs, s->bitmap_cache, BLOCK_TABLE_BITMAP,
>>> + ADD_COW_CACHE_ENTRY_SIZE);
>>> + qemu_co_mutex_unlock(&s->lock);
>>> + return ret;
>>> +}
>>
>> What about flushing s->image_file?
>>> +typedef struct AddCowHeader {
>>> + uint64_t magic;
>>> + uint32_t version;
>>> +
>>> + uint32_t backing_filename_offset;
>>> + uint32_t backing_filename_size;
>>> +
>>> + uint32_t image_filename_offset;
>>> + uint32_t image_filename_size;
>>> +
>>> + uint64_t features;
>>> + uint64_t optional_features;
>>> + uint32_t header_pages_size;
>>> +} QEMU_PACKED AddCowHeader;
>>
>> Why aren't backing/image_file_format part of the header here? They are
>> in the spec. It would also simplify some offset calculation code.
>>
>
> Anthony said "It's far better to shrink the size of the header and use
> an offset/len
> pointer to the backing file string. Limiting backing files to 1023 is
> unacceptable"
>
> http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg04110.html
>
> So I use offset and length instead of using string directly.
I'm talking about the format, not the path.
Kevin
