From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XW2YQ-0003ZB-NW for qemu-devel@nongnu.org; Mon, 22 Sep 2014 08:17:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XW2YL-000174-Dg for qemu-devel@nongnu.org; Mon, 22 Sep 2014 08:17:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9429) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XW2YL-00014i-60 for qemu-devel@nongnu.org; Mon, 22 Sep 2014 08:17:13 -0400 Date: Mon, 22 Sep 2014 13:17:00 +0100 From: Stefan Hajnoczi Message-ID: <20140922121700.GD12513@stefanha-thinkpad.redhat.com> References: <1411120786-28764-1-git-send-email-lpetrut@cloudbasesolutions.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="zS7rBR6csb6tI2e1" Content-Disposition: inline In-Reply-To: <1411120786-28764-1-git-send-email-lpetrut@cloudbasesolutions.com> Subject: Re: [Qemu-devel] [[PATCH v2] 1/1] vpc.c: Add VHD resize support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lucian Petrut Cc: kwolf@redhat.com, Lucian Petrut , sw@weilnetz.de, jcody@redhat.com, qemu-devel@nongnu.org, apilotti@cloudbasesolutions.com --zS7rBR6csb6tI2e1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > +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() > + 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 > + > + 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). 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 > + 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.) > + } > + > + 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; > + 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. > + 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? > + } > + > + 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() --zS7rBR6csb6tI2e1 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUIBM8AAoJEJykq7OBq3PIseMIAIXPiAicztjG+1ORA3oaPifk jg8PHU1Ao4r5gz8MI/gE0AOz85e8UnxrIBpXTghPPzgSl1Cx+G3yzz0kkiINw1SI iEwwdYpEcvY/tly6vcZ2cCO0O5/na0OuHSf9ddlq2GGba1AxbLvvRzBuDyybxcmL YkQg1GSD7sXdRebWrZgyEIG117ZUhXi2TmthVwWuEgzH/l74od8I5WOmen0ZdIrd hO6BwQpmjanoprpGnh/4M7s1jh4Nh7iIjgeClxrRBi0hij2LeGWIGFvDeHOVLZ+j oSlBAvCJdecvWbTz/1zp8kIFEQ2Yoo/iIoDq3wcdVgg7amZ7l/eO8MinfvObtmg= =87Ww -----END PGP SIGNATURE----- --zS7rBR6csb6tI2e1--