On 2015-02-24 at 01:49, Peter Lieven wrote:
Am 23.02.2015 um 19:59 schrieb Max Reitz:
On 2015-02-23 at 09:27, Peter Lieven wrote:
The VHD spec [1] allows for total_sectors of 65535 x 16 x 255 (~127GB)
represented by a CHS geometry. If total_sectors is greater
than 65535 x 16 x 255 this geometry is set as a maximum.
Qemu, Hyper-V, VirtualBox and disk2vhd use this special geometry as an
indicator to use the image current size from the footer as disk size.
This patch changes vpc_create to effectively calculate a CxHxS geometry
for the given image size if possible while rounding up if necessary.
If the image size is too big to be represented in CHS we set the
maximum
and write the exact requested image size into the footer.
This partly reverts commit 258d2edb, but leaves support for >127G disks
intact.
[1]
http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc
Signed-off-by: Peter Lieven <p...@kamp.de>
---
block/vpc.c | 45 ++++++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index 11d3c86..9c5301b 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -46,6 +46,7 @@ enum vhd_type {
#define VHD_TIMESTAMP_BASE 946684800
#define VHD_MAX_SECTORS (65535LL * 255 * 255)
+#define VHD_MAX_GEOMETRY (65535LL * 16 * 255)
// always big-endian
typedef struct vhd_footer {
@@ -218,7 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict
*options, int flags,
/* Images that have exactly the maximum geometry are probably
bigger and
* would be truncated if we adhered to the geometry for them.
Rely on
* footer->size for them. */
- if (bs->total_sectors == 65535ULL * 16 * 255) {
+ if (bs->total_sectors == VHD_MAX_GEOMETRY) {
bs->total_sectors = be64_to_cpu(footer->size) /
BDRV_SECTOR_SIZE;
}
@@ -642,26 +643,20 @@ static int calculate_geometry(int64_t
total_sectors, uint16_t* cyls,
{
uint32_t cyls_times_heads;
- /* Allow a maximum disk size of approximately 2 TB */
- if (total_sectors > 65535LL * 255 * 255) {
- return -EFBIG;
- }
+ total_sectors = MIN(total_sectors, VHD_MAX_GEOMETRY);
- if (total_sectors > 65535 * 16 * 63) {
+ if (total_sectors > 65535LL * 16 * 63) {
While it is >= in the specification, the "else" branch does work fine
with the total_sectors = 65535 * 16 * 63 case. So I'm leaving it up
to you whether to make it really really spec-conform or at least
functionally spec-conform (although the spec says 16 heads, 16191
cylinders, and 255 sectors per cylinder for this case, whereas the
"else" branch leaves us with 16 heads, 65535 cylinders, and 63
sectors per cylinder).
I will adjust it to match the spec exactly.
*secs_per_cyl = 255;
- if (total_sectors > 65535 * 16 * 255) {
- *heads = 255;
- } else {
- *heads = 16;
- }
+ *heads = 16;
cyls_times_heads = total_sectors / *secs_per_cyl;
} else {
*secs_per_cyl = 17;
cyls_times_heads = total_sectors / *secs_per_cyl;
*heads = (cyls_times_heads + 1023) / 1024;
- if (*heads < 4)
+ if (*heads < 4) {
*heads = 4;
+ }
if (cyls_times_heads >= (*heads * 1024) || *heads > 16) {
*secs_per_cyl = 31;
@@ -817,19 +812,27 @@ static int vpc_create(const char *filename,
QemuOpts *opts, Error **errp)
* Calculate matching total_size and geometry. Increase the
number of
* sectors requested until we get enough (or fail). This
ensures that
* qemu-img convert doesn't truncate images, but rather rounds
up.
+ *
+ * If the image size can't be represented by a spec conform CHS
geometry,
+ * we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use
+ * the image size from the VHD footer to calculate total_sectors.
*/
- total_sectors = total_size / BDRV_SECTOR_SIZE;
+ total_sectors = MIN(VHD_MAX_GEOMETRY,
+ DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE));
DIV_ROUND_UP() is unnecessary, total_size is a multiple of
BDRV_SECTOR_SIZE anyway.
ok
I find the MIN() unnecessary, too, because calculate_geometry() will
take care of that itself.
Actually the MIN() is the guarantee that the for-loop terminates for
images with total_sectors > VHD_MAX_GEOMETRY.
Otherwise (int64_t)cyls * heads * secs_per_cyl would always be
VHD_MAX_GEOMETRY which is always
smaller than VHD_MAX_GEOMETRY. This would keep us looping for good.
Right. Very well then.
for (i = 0; total_sectors > (int64_t)cyls * heads *
secs_per_cyl; i++) {
- if (calculate_geometry(total_sectors + i, &cyls, &heads,
- &secs_per_cyl))
- {
- ret = -EFBIG;
- goto out;
- }
+ calculate_geometry(total_sectors + i, &cyls, &heads,
&secs_per_cyl);
}
- total_sectors = (int64_t) cyls * heads * secs_per_cyl;
- total_size = total_sectors * BDRV_SECTOR_SIZE;
+ if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) {
+ total_sectors = DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE);
Again, DIV_ROUND_UP() is unnecessary.
So this is equivalent to total_sectors = total_size /
BDRV_SECTOR_SIZE, which, if you omit the MIN() above, is exactly what
total_sectors already contains, so you can just omit this assignment.
+ /* Allow a maximum disk size of approximately 2 TB */
+ if (total_sectors > VHD_MAX_SECTORS) {
+ return -EFBIG;
No goto out;?
Oops.
+ }
+ } else {
+ total_sectors = (int64_t)cyls * heads * secs_per_cyl;
+ total_size = total_sectors * BDRV_SECTOR_SIZE;
+ }
An alternative way of writing this would be:
if ((int64_t)cyls * heads * secs_per_cyl < VHD_MAX_GEOMETRY) {
total_sectors = (int64_t)cyls * heads * secs_per_cyl;
}
if (total_sectors > VHD_MAX_SECTORS) {
ret = -EFBIG;
goto out;
}
total_size = total_sectors * BDRV_SECTOR_SIZE;
Or, alternatively, it'd be possible to drop the total_size assignment
and just use total_sectors * BDRV_SECTOR_SIZE in the assignments of
footer->orig_size and footer->size.
Will look at this.
Well, it won't work so well with the MIN() being necessary...
Max
