Thanks for reviewing this patch! I’ll send a new one, fixing the issues you’ve pointed out, along with some tests.
Please see my inline comments bellow. Regards, Lucian Petrut >From: Stefan Hajnoczi >Sent: Monday, September 22, 2014 3:17 PM >To: Petrut Lucian >Cc: qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org>, Lucian Petrut, >jc...@redhat.com<mailto:jc...@redhat.com>, >kw...@redhat.com<mailto:kw...@redhat.com>, >s...@weilnetz.de<mailto:s...@weilnetz.de>, Alessandro Pilotti > > >On Fri, Sep 19, 2014 at 12:59:46PM +0300, Lucian Petrut wrote: >> Note that this patch assumes that all the data blocks are written >> right after the BAT. >Are there any known files out there where this is not the case? >Perhaps an error should be printed if a data block that requires moving >does not appear in pagetable[]. That's an indication that the image is >not laid out as expected. You are right, a check should be added here. I’m not aware of parsers that may have this behavior but better be on the safe side. >> +static int vpc_truncate(BlockDriverState *bs, int64_t offset) >> +{ >> + BDRVVPCState *s = bs->opaque; >> + VHDFooter *footer = (VHDFooter *) s->footer_buf; >> + VHDDynDiskHeader *dyndisk_header; >> + void *buf = NULL; >> + int64_t new_total_sectors, old_bat_size, new_bat_size, >> + block_offset, new_block_offset, bat_offset; >> + int32_t bat_value, data_blocks_required; >> + int ret = 0; >> + uint16_t cyls = 0; >> + uint8_t heads = 0; >> + uint8_t secs_per_cyl = 0; >> + uint32_t new_num_bat_entries; >> + uint64_t index, block_index, new_bat_right_limit; >> + >> + if (offset & 511) { >> + error_report("The new size must be a multiple of 512."); >> + return -EINVAL; >> + } >> + >> + if (offset < bs->total_sectors * 512) { >> + error_report("Shrinking vhd images is not supported."); >> + return -ENOTSUP; >> + } >> + >> + if (cpu_to_be32(footer->type) == VHD_DIFFERENCING) { >footer_buf is big-endian so this should be be32_to_cpu() My bad, I’ll fix the BE related issues. >> + error_report("Resizing differencing vhd images is not supported."); >> + return -ENOTSUP; >> + } >> + >> + old_bat_size = (s->max_table_entries * 4 + 511) & ~511; >> + new_total_sectors = offset / BDRV_SECTOR_SIZE; >> + >> + for (index = 0; new_total_sectors > (int64_t)cyls * heads * >> secs_per_cyl; >> + index++) { >> + if (calculate_geometry(new_total_sectors + index, &cyls, &heads, >> + &secs_per_cyl)) { >> + return -EFBIG; >> + } >> + } >> + new_total_sectors = (int64_t) cyls * heads * secs_per_cyl; >> + new_num_bat_entries = (new_total_sectors + s->block_size / 512) / >> + (s->block_size / 512); >This expression doesn't just round up, it always adds an extra block. >Is this intentional? >I expected the numerator for rounding up to be: > new_total_sectors + s->block_size / 512 - 1 This was intended, it may just add an extra entry. Also, the number of entries is calculated in the same way in the existing code which creates dynamic VHDs, so I wanted to keep consistency with that as well. >> + >> + if (cpu_to_be32(footer->type) == VHD_DYNAMIC) { >be32_to_cpu() >> + new_bat_size = (new_num_bat_entries * 4 + 511) & ~511; >> + /* Number of blocks required for extending the BAT */ >> + data_blocks_required = (new_bat_size - old_bat_size + >> + s->block_size - 1) / s->block_size; >> + new_bat_right_limit = s->bat_offset + old_bat_size + >> + data_blocks_required * >> + (s->block_size + s->bitmap_size); >> + >> + for (block_index = 0; block_index < >> + data_blocks_required; block_index++){ >> + /* >> + * The BAT has to be extended. We'll have to move the first >> + * data block(s) to the end of the file, making room for the >> + * BAT to expand. Also, the BAT entries have to be updated for >> + * the moved blocks. >> + */ >> + >> + block_offset = s->bat_offset + old_bat_size + >> + block_index * (s->block_size + s->bitmap_size); >> + if (block_offset >= s->free_data_block_offset) { >> + /* >> + * Do not allocate a new block for the BAT if no data blocks >> + * were previously allocated to the vhd image. >> + */ >> + s->free_data_block_offset += (new_bat_size - old_bat_size); >> + break; >> + } >> + >> + if (block_index == 0) { >Is this condition correct? I expected !buf. What happens during >the second loop iteration when block_index == 1 (we freed buf at the end >of the first iteration). Whops, my bad. You are right, I should just use !buf. At first, I did not free this at the end of iteration so I forgot to change this as well. >If I'm reading the code correctly this results in a NULL pointer >dereference. That would mean you never executed this code path. >Please create a qemu-iotests test case to exercise the code paths in >your patch. There are many examples you can use as a starting point in >tests/qemu-iotests/. The basic overview is here: >http://qemu-project.org/Documentation/QemuIoTests Thanks for the link, I’ll add a test for this. >> + buf = g_malloc(s->block_size + s->bitmap_size); >Please use qemu_blockalign()/qemu_vfree() for disk data buffers. If the >file is opened with O_DIRECT there are memory alignment constraints. >Using qemu_blockalign() avoids the need for a bounce buffer in >block/raw-posix.c. >(Even if there is no performance concern, it's a good habit to follow >consistently so that we do align in the cases where the performance does >matter.) You are right, this would be much better. >> + } >> + >> + ret = bdrv_pread(bs->file, block_offset, buf, >> + s->block_size + s->bitmap_size); >> + if (ret < 0) { >> + goto out; >> + } >> + >> + new_block_offset = s->free_data_block_offset < >> new_bat_right_limit ? >> + new_bat_right_limit : s->free_data_block_offset; >> + bdrv_pwrite_sync(bs->file, new_block_offset, buf, >> + s->block_size + s->bitmap_size); >> + if (ret < 0) { >> + goto out; >> + } >> + >> + for (index = 0; index < s->max_table_entries; index++) { >> + if (s->pagetable[index] == block_offset / >> BDRV_SECTOR_SIZE) { >> + s->pagetable[index] = block_offset; >The if condition checks for block_offset / BDRV_SECTOR_SIZE, but the >assignment stores block_offset (without converting it to sectors). >Did you mean: > if (s->pagetable[index] == block_offset / BDRV_SECTOR_SIZE) { > s->pagetable[index] = new_block_offset / BDRV_SECTOR_SIZE; You are right here as well. I’ve actually done it this way but forgot to amend the commit, thanks for pointing this out! >> + bat_offset = s->bat_offset + (4 * index); >> + bat_value = be32_to_cpu(new_block_offset / >> + BDRV_SECTOR_SIZE); >cpu_to_be32() >> + ret = bdrv_pwrite_sync(bs->file, bat_offset, >> &bat_value, 4); >> + if (ret < 0) { >> + goto out; >> + } >> + break; >> + } >> + } >> + >> + s->free_data_block_offset = new_block_offset + s->block_size + >> + s->bitmap_size; >> + g_free(buf); >> + buf = NULL; >> + } >> + >> + buf = g_malloc(512); >> + memset(buf, 0xFF, 512); >> + >> + /* Extend the BAT */ >> + offset = s->bat_offset + old_bat_size; >> + for (index = 0; >> + index < (new_bat_size - old_bat_size) / 512; >> + index++) { >> + ret = bdrv_pwrite_sync(bs->file, offset, buf, 512); >You could just bdrv_pwrite() and bdrv_flush() after the loop to make >this faster. Done. >> + if (ret < 0) { >> + goto out; >> + } >> + offset += 512; >> + } >> + >> + g_free(buf); >> + buf = g_malloc(1024); >> + >> + /* Update the Dynamic Disk Header */ >> + ret = bdrv_pread(bs->file, 512, buf, >> + 1024); >> + if (ret < 0) { >> + goto out; >> + } >> + >> + dyndisk_header = (VHDDynDiskHeader *) buf; >> + dyndisk_header->max_table_entries = >> be32_to_cpu(new_num_bat_entries); >cpu_to_be32() >> + dyndisk_header->checksum = 0; >> + dyndisk_header->checksum = be32_to_cpu(vpc_checksum(buf, 1024)); >cpu_to_be32() >> + ret = bdrv_pwrite_sync(bs->file, 512, buf, 1024); >> + if (ret < 0) { >> + goto out; >> + } >> + >> + } else { >> + s->free_data_block_offset = new_total_sectors * BDRV_SECTOR_SIZE; >Don't we have to take into account the header for VHD_FIXED? Fixed VHDs, unlike the other VHD types, don’t have a copy of the footer in the file header, nor do they have the “dynamic vhd header”. >> + } >> + >> + footer->cyls = be16_to_cpu(cyls); >cpu_to_be16() >> + footer->heads = heads; >> + footer->secs_per_cyl = secs_per_cyl; >> + footer->size = be64_to_cpu(new_total_sectors * BDRV_SECTOR_SIZE); >cpu_to_be64() >> + footer->checksum = 0; >> + footer->checksum = be32_to_cpu(vpc_checksum(s->footer_buf, >> HEADER_SIZE)); >cpu_to_be32() >> + >> + /* >> + * Rewrite the footer, copying to the image header in case of a >> + * dynamic vhd. >> + */ >> + rewrite_footer(bs, (cpu_to_be32(footer->type) != VHD_FIXED)); >be32_to_cpu()