* [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
@ 2012-01-31 19:03 Charles Arnold
2012-01-31 22:08 ` Andreas Färber
0 siblings, 1 reply; 14+ messages in thread
From: Charles Arnold @ 2012-01-31 19:03 UTC (permalink / raw)
To: qemu-devel
The Virtual Hard Disk Image Format Specification allows for three
types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
currently only supports Dynamic disks. This patch adds support for
the Fixed Disk format.
Usage:
Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output filename>
While it is also allowed to specify '-o type=dynamic', the default disk type
remains Dynamic and is what is used when the type is left unspecified.
Signed-off-by: Charles Arnold <carnold@suse.com>
diff --git a/block/vpc.c b/block/vpc.c
index 89a5ee2..bfe5f7b 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -160,6 +160,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
struct vhd_dyndisk_header* dyndisk_header;
uint8_t buf[HEADER_SIZE];
uint32_t checksum;
+ int disk_type = VHD_DYNAMIC;
int err = -1;
if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
@@ -167,7 +168,16 @@ static int vpc_open(BlockDriverState *bs, int flags)
footer = (struct vhd_footer*) s->footer_buf;
if (strncmp(footer->creator, "conectix", 8))
- goto fail;
+ {
+ int64_t offset = bdrv_getlength(bs->file);
+ // If a fixed disk, the footer is found only at the end of the file
+ if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
+ != HEADER_SIZE)
+ goto fail;
+ if (strncmp(footer->creator, "conectix", 8))
+ goto fail;
+ disk_type = VHD_FIXED;
+ }
checksum = be32_to_cpu(footer->checksum);
footer->checksum = 0;
@@ -186,6 +196,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
goto fail;
}
+ // The footer is all that is needed for fixed disks.
+ if (disk_type == VHD_FIXED) {
+ // The fixed disk format doesn't use footer->data_offset but it
+ // should be initialized
+ footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFF);
+ return 0;
+ }
+
if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
!= HEADER_SIZE)
goto fail;
@@ -533,7 +551,7 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
return 0;
}
-static int vpc_create(const char *filename, QEMUOptionParameter *options)
+static int vpc_create_dynamic_disk(const char *filename, int64_t total_size)
{
uint8_t buf[1024];
struct vhd_footer* footer = (struct vhd_footer*) buf;
@@ -544,13 +562,9 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
uint8_t heads = 0;
uint8_t secs_per_cyl = 0;
size_t block_size, num_bat_entries;
- int64_t total_sectors = 0;
+ int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
int ret = -EIO;
- // Read out options
- total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
- BDRV_SECTOR_SIZE;
-
// Create the file
fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
if (fd < 0)
@@ -657,6 +671,101 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
return ret;
}
+static int vpc_create_fixed_disk(const char *filename, int64_t total_size)
+{
+ uint8_t buf[1024];
+ struct vhd_footer* footer = (struct vhd_footer*) buf;
+ int fd;
+ uint16_t cyls = 0;
+ uint8_t heads = 0;
+ uint8_t secs_per_cyl = 0;
+ int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
+ int ret = -EIO;
+
+ // Create the file
+ fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
+ if (fd < 0)
+ return -EIO;
+
+ /* Calculate matching total_size and geometry. */
+ if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
+ ret = -EFBIG;
+ goto fail;
+ }
+ total_sectors = (int64_t) cyls * heads * secs_per_cyl;
+
+ // Prepare the Hard Disk Footer
+ memset(buf, 0, 1024);
+
+ memcpy(footer->creator, "conectix", 8);
+ // TODO Check if "qemu" creator_app is ok for VPC
+ memcpy(footer->creator_app, "qemu", 4);
+ memcpy(footer->creator_os, "Wi2k", 4);
+
+ footer->features = be32_to_cpu(0x02);
+ footer->version = be32_to_cpu(0x00010000);
+ footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFF);
+ footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
+
+ // Version of Virtual PC 2007
+ footer->major = be16_to_cpu(0x0005);
+ footer->minor =be16_to_cpu(0x0003);
+ footer->orig_size = be64_to_cpu(total_size);
+ footer->size = be64_to_cpu(total_size);
+ footer->cyls = be16_to_cpu(cyls);
+ footer->heads = heads;
+ footer->secs_per_cyl = secs_per_cyl;
+
+ footer->type = be32_to_cpu(VHD_FIXED);
+
+ // TODO uuid is missing
+
+ footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
+
+ total_size += 512;
+ if (ftruncate(fd, total_size) != 0) {
+ ret = -errno;
+ goto fail;
+ }
+ if (lseek(fd, -512, SEEK_END) < 0) {
+ goto fail;
+ }
+ if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
+ goto fail;
+ }
+
+ ret = 0;
+
+ fail:
+ close(fd);
+ return ret;
+}
+
+static int vpc_create(const char *filename, QEMUOptionParameter *options)
+{
+ int64_t total_size = 0;
+ QEMUOptionParameter *disk_type_param;
+ int ret = -EIO;
+
+ // Read out options
+ total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
+
+ disk_type_param = get_option_parameter(options, BLOCK_OPT_TYPE);
+ if (disk_type_param && disk_type_param->value.s) {
+ if (!strcmp(disk_type_param->value.s, "dynamic") ) {
+ ret = vpc_create_dynamic_disk(filename, total_size);
+ }
+ else if (!strcmp(disk_type_param->value.s, "fixed") ) {
+ ret = vpc_create_fixed_disk(filename, total_size);
+ }
+ else
+ ret = -EINVAL;
+ } else {
+ ret = vpc_create_dynamic_disk(filename, total_size);
+ }
+ return ret;
+}
+
static void vpc_close(BlockDriverState *bs)
{
BDRVVPCState *s = bs->opaque;
@@ -675,6 +784,13 @@ static QEMUOptionParameter vpc_create_options[] = {
.type = OPT_SIZE,
.help = "Virtual disk size"
},
+ {
+ .name = BLOCK_OPT_TYPE,
+ .type = OPT_STRING,
+ .help =
+ "Type of virtual hard disk format. Supported formats are "
+ "{dynamic (default) | fixed} "
+ },
{ NULL }
};
diff --git a/block_int.h b/block_int.h
index 311bd2a..6d6cc96 100644
--- a/block_int.h
+++ b/block_int.h
@@ -50,6 +50,7 @@
#define BLOCK_OPT_TABLE_SIZE "table_size"
#define BLOCK_OPT_PREALLOC "preallocation"
#define BLOCK_OPT_SUBFMT "subformat"
+#define BLOCK_OPT_TYPE "type"
typedef struct BdrvTrackedRequest BdrvTrackedRequest;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-01-31 19:03 [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type Charles Arnold
@ 2012-01-31 22:08 ` Andreas Färber
2012-01-31 23:04 ` Charles Arnold
0 siblings, 1 reply; 14+ messages in thread
From: Andreas Färber @ 2012-01-31 22:08 UTC (permalink / raw)
To: Charles Arnold; +Cc: Kevin Wolf, qemu-devel
Hello Charles,
Am 31.01.2012 20:03, schrieb Charles Arnold:
> The Virtual Hard Disk Image Format Specification allows for three
> types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
> currently only supports Dynamic disks. This patch adds support for
> the Fixed Disk format.
>
> Usage:
> Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
> Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output filename>
>
> While it is also allowed to specify '-o type=dynamic', the default disk type
> remains Dynamic and is what is used when the type is left unspecified.
>
> Signed-off-by: Charles Arnold <carnold@suse.com>
Please cc the block maintainer. If --cc-cmd=scripts/get_maintainer.pl
doesn't pick him up please let us know so that we can fix that.
Many Coding Style issues (most probably from the duplicated code?), two
32-bit issues, otherwise looks okay.
The patch itself arrived with lots of trailing whitespace in some places
though.
Please run scripts/checkpatch.pl. That should notify you of most issues
noted below before submitting, see for instance:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html
> diff --git a/block/vpc.c b/block/vpc.c
> index 89a5ee2..bfe5f7b 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -160,6 +160,7 @@ static int vpc_open(BlockDriverState *bs, int flags)
> struct vhd_dyndisk_header* dyndisk_header;
> uint8_t buf[HEADER_SIZE];
> uint32_t checksum;
> + int disk_type = VHD_DYNAMIC;
> int err = -1;
>
> if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
> @@ -167,7 +168,16 @@ static int vpc_open(BlockDriverState *bs, int flags)
>
> footer = (struct vhd_footer*) s->footer_buf;
> if (strncmp(footer->creator, "conectix", 8))
> - goto fail;
> + {
Brace on the previous line please.
> + int64_t offset = bdrv_getlength(bs->file);
> + // If a fixed disk, the footer is found only at the end of the file
Please use ANSI C comments /* ... */ exclusively.
> + if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
> + != HEADER_SIZE)
> + goto fail;
Always braces for if.
> + if (strncmp(footer->creator, "conectix", 8))
> + goto fail;
Dito.
> + disk_type = VHD_FIXED;
> + }
>
> checksum = be32_to_cpu(footer->checksum);
> footer->checksum = 0;
> @@ -186,6 +196,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
> goto fail;
> }
>
> + // The footer is all that is needed for fixed disks.
/* ... */
> + if (disk_type == VHD_FIXED) {
> + // The fixed disk format doesn't use footer->data_offset but it
> + // should be initialized
/* ... */
> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFF);
For 32-bit hosts 64-bit constants need ULL suffix.
> + return 0;
> + }
> +
> if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
> != HEADER_SIZE)
> goto fail;
> @@ -533,7 +551,7 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
> return 0;
> }
>
> -static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +static int vpc_create_dynamic_disk(const char *filename, int64_t total_size)
> {
> uint8_t buf[1024];
> struct vhd_footer* footer = (struct vhd_footer*) buf;
> @@ -544,13 +562,9 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
> uint8_t heads = 0;
> uint8_t secs_per_cyl = 0;
> size_t block_size, num_bat_entries;
> - int64_t total_sectors = 0;
> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
> int ret = -EIO;
>
> - // Read out options
> - total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
> - BDRV_SECTOR_SIZE;
> -
> // Create the file
> fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> if (fd < 0)
> @@ -657,6 +671,101 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
> return ret;
> }
>
> +static int vpc_create_fixed_disk(const char *filename, int64_t total_size)
> +{
> + uint8_t buf[1024];
> + struct vhd_footer* footer = (struct vhd_footer*) buf;
> + int fd;
> + uint16_t cyls = 0;
> + uint8_t heads = 0;
> + uint8_t secs_per_cyl = 0;
> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
> + int ret = -EIO;
> +
> + // Create the file
/* ... */
> + fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> + if (fd < 0)
Braces. I'll stop here.
> + return -EIO;
> +
> + /* Calculate matching total_size and geometry. */
> + if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
> + ret = -EFBIG;
> + goto fail;
> + }
> + total_sectors = (int64_t) cyls * heads * secs_per_cyl;
> +
> + // Prepare the Hard Disk Footer
> + memset(buf, 0, 1024);
> +
> + memcpy(footer->creator, "conectix", 8);
> + // TODO Check if "qemu" creator_app is ok for VPC
> + memcpy(footer->creator_app, "qemu", 4);
> + memcpy(footer->creator_os, "Wi2k", 4);
> +
> + footer->features = be32_to_cpu(0x02);
> + footer->version = be32_to_cpu(0x00010000);
> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFF);
ULL
> + footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
> +
> + // Version of Virtual PC 2007
> + footer->major = be16_to_cpu(0x0005);
> + footer->minor =be16_to_cpu(0x0003);
Space after = please.
> + footer->orig_size = be64_to_cpu(total_size);
> + footer->size = be64_to_cpu(total_size);
> + footer->cyls = be16_to_cpu(cyls);
> + footer->heads = heads;
> + footer->secs_per_cyl = secs_per_cyl;
> +
> + footer->type = be32_to_cpu(VHD_FIXED);
> +
> + // TODO uuid is missing
Do you know what exactly it means, or did you just copy it?
> +
> + footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
> +
> + total_size += 512;
> + if (ftruncate(fd, total_size) != 0) {
> + ret = -errno;
> + goto fail;
> + }
> + if (lseek(fd, -512, SEEK_END) < 0) {
> + goto fail;
> + }
> + if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
> + goto fail;
> + }
> +
> + ret = 0;
> +
> + fail:
> + close(fd);
> + return ret;
> +}
> +
> +static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +{
> + int64_t total_size = 0;
> + QEMUOptionParameter *disk_type_param;
> + int ret = -EIO;
> +
> + // Read out options
> + total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
> +
> + disk_type_param = get_option_parameter(options, BLOCK_OPT_TYPE);
> + if (disk_type_param && disk_type_param->value.s) {
> + if (!strcmp(disk_type_param->value.s, "dynamic") ) {
> + ret = vpc_create_dynamic_disk(filename, total_size);
> + }
> + else if (!strcmp(disk_type_param->value.s, "fixed") ) {
else on same line as } please. Also superfluous spaces after "dynamic")
and "fixed").
> + ret = vpc_create_fixed_disk(filename, total_size);
> + }
> + else
> + ret = -EINVAL;
> + } else {
> + ret = vpc_create_dynamic_disk(filename, total_size);
> + }
> + return ret;
> +}
> +
> static void vpc_close(BlockDriverState *bs)
> {
> BDRVVPCState *s = bs->opaque;
> @@ -675,6 +784,13 @@ static QEMUOptionParameter vpc_create_options[] = {
> .type = OPT_SIZE,
> .help = "Virtual disk size"
> },
> + {
> + .name = BLOCK_OPT_TYPE,
> + .type = OPT_STRING,
> + .help =
> + "Type of virtual hard disk format. Supported formats are "
> + "{dynamic (default) | fixed} "
> + },
> { NULL }
> };
>
> diff --git a/block_int.h b/block_int.h
> index 311bd2a..6d6cc96 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -50,6 +50,7 @@
> #define BLOCK_OPT_TABLE_SIZE "table_size"
> #define BLOCK_OPT_PREALLOC "preallocation"
> #define BLOCK_OPT_SUBFMT "subformat"
> +#define BLOCK_OPT_TYPE "type"
>
> typedef struct BdrvTrackedRequest BdrvTrackedRequest;
>
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-01-31 22:08 ` Andreas Färber
@ 2012-01-31 23:04 ` Charles Arnold
2012-01-31 23:15 ` Andreas Färber
2012-02-01 12:15 ` Kevin Wolf
0 siblings, 2 replies; 14+ messages in thread
From: Charles Arnold @ 2012-01-31 23:04 UTC (permalink / raw)
To: Andreas Färber; +Cc: Kevin Wolf, qemu-devel
Thanks Andreas,
The 'TODO uuid is missing' comment in the patch is from the
original sources (as well as many '//' comments). The vhd footer
and header data structures contain a field for a UUID but no code
was ever developed to generate one.
The revised patch is below after running scripts/checkpatch.pl and
fixing the 32 bit issues.
- Charles
The Virtual Hard Disk Image Format Specification allows for three
types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
currently only supports Dynamic disks. This patch adds support for
the Fixed Disk format.
Usage:
Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output filename>
While it is also allowed to specify '-o type=dynamic', the default disk type
remains Dynamic and is what is used when the type is left unspecified.
Signed-off-by: Charles Arnold <carnold@suse.com>
diff --git a/block/vpc.c b/block/vpc.c
index 89a5ee2..04db372 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -160,14 +160,25 @@ static int vpc_open(BlockDriverState *bs, int flags)
struct vhd_dyndisk_header* dyndisk_header;
uint8_t buf[HEADER_SIZE];
uint32_t checksum;
+ int disk_type = VHD_DYNAMIC;
int err = -1;
if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
goto fail;
footer = (struct vhd_footer*) s->footer_buf;
- if (strncmp(footer->creator, "conectix", 8))
- goto fail;
+ if (strncmp(footer->creator, "conectix", 8)) {
+ int64_t offset = bdrv_getlength(bs->file);
+ /* If a fixed disk, the footer is found only at the end of the file */
+ if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
+ != HEADER_SIZE) {
+ goto fail;
+ }
+ if (strncmp(footer->creator, "conectix", 8)) {
+ goto fail;
+ }
+ disk_type = VHD_FIXED;
+ }
checksum = be32_to_cpu(footer->checksum);
footer->checksum = 0;
@@ -186,6 +197,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
goto fail;
}
+ /* The footer is all that is needed for fixed disks */
+ if (disk_type == VHD_FIXED) {
+ /* The fixed disk format doesn't use footer->data_offset but it
+ should be initialized */
+ footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
+ return 0;
+ }
+
if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
!= HEADER_SIZE)
goto fail;
@@ -533,10 +552,10 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
return 0;
}
-static int vpc_create(const char *filename, QEMUOptionParameter *options)
+static int vpc_create_dynamic_disk(const char *filename, int64_t total_size)
{
uint8_t buf[1024];
- struct vhd_footer* footer = (struct vhd_footer*) buf;
+ struct vhd_footer* footer = (struct vhd_footer *) buf;
struct vhd_dyndisk_header* dyndisk_header =
(struct vhd_dyndisk_header*) buf;
int fd, i;
@@ -544,13 +563,9 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
uint8_t heads = 0;
uint8_t secs_per_cyl = 0;
size_t block_size, num_bat_entries;
- int64_t total_sectors = 0;
+ int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
int ret = -EIO;
- // Read out options
- total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
- BDRV_SECTOR_SIZE;
-
// Create the file
fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
if (fd < 0)
@@ -657,6 +672,101 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
return ret;
}
+static int vpc_create_fixed_disk(const char *filename, int64_t total_size)
+{
+ uint8_t buf[1024];
+ struct vhd_footer* footer = (struct vhd_footer *) buf;
+ int fd;
+ uint16_t cyls = 0;
+ uint8_t heads = 0;
+ uint8_t secs_per_cyl = 0;
+ int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
+ int ret = -EIO;
+
+ /* Create the file */
+ fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
+ if (fd < 0) {
+ return -EIO;
+ }
+
+ /* Calculate matching total_size and geometry. */
+ if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
+ ret = -EFBIG;
+ goto fail;
+ }
+ total_sectors = (int64_t) cyls * heads * secs_per_cyl;
+
+ /* Prepare the Hard Disk Footer */
+ memset(buf, 0, 1024);
+
+ memcpy(footer->creator, "conectix", 8);
+ /* TODO Check if "qemu" creator_app is ok for VPC */
+ memcpy(footer->creator_app, "qemu", 4);
+ memcpy(footer->creator_os, "Wi2k", 4);
+
+ footer->features = be32_to_cpu(0x02);
+ footer->version = be32_to_cpu(0x00010000);
+ footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
+ footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
+
+ /* Version of Virtual PC 2007 */
+ footer->major = be16_to_cpu(0x0005);
+ footer->minor = be16_to_cpu(0x0003);
+ footer->orig_size = be64_to_cpu(total_size);
+ footer->size = be64_to_cpu(total_size);
+ footer->cyls = be16_to_cpu(cyls);
+ footer->heads = heads;
+ footer->secs_per_cyl = secs_per_cyl;
+
+ footer->type = be32_to_cpu(VHD_FIXED);
+
+ /* TODO uuid is missing */
+
+ footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
+
+ total_size += 512;
+ if (ftruncate(fd, total_size) != 0) {
+ ret = -errno;
+ goto fail;
+ }
+ if (lseek(fd, -512, SEEK_END) < 0) {
+ goto fail;
+ }
+ if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
+ goto fail;
+ }
+
+ ret = 0;
+
+ fail:
+ close(fd);
+ return ret;
+}
+
+static int vpc_create(const char *filename, QEMUOptionParameter *options)
+{
+ int64_t total_size = 0;
+ QEMUOptionParameter *disk_type_param;
+ int ret = -EIO;
+
+ /* Read out options */
+ total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
+
+ disk_type_param = get_option_parameter(options, BLOCK_OPT_TYPE);
+ if (disk_type_param && disk_type_param->value.s) {
+ if (!strcmp(disk_type_param->value.s, "dynamic")) {
+ ret = vpc_create_dynamic_disk(filename, total_size);
+ } else if (!strcmp(disk_type_param->value.s, "fixed")) {
+ ret = vpc_create_fixed_disk(filename, total_size);
+ } else {
+ ret = -EINVAL;
+ }
+ } else {
+ ret = vpc_create_dynamic_disk(filename, total_size);
+ }
+ return ret;
+}
+
static void vpc_close(BlockDriverState *bs)
{
BDRVVPCState *s = bs->opaque;
@@ -675,6 +785,13 @@ static QEMUOptionParameter vpc_create_options[] = {
.type = OPT_SIZE,
.help = "Virtual disk size"
},
+ {
+ .name = BLOCK_OPT_TYPE,
+ .type = OPT_STRING,
+ .help =
+ "Type of virtual hard disk format. Supported formats are "
+ "{dynamic (default) | fixed} "
+ },
{ NULL }
};
diff --git a/block_int.h b/block_int.h
index 311bd2a..6d6cc96 100644
--- a/block_int.h
+++ b/block_int.h
@@ -50,6 +50,7 @@
#define BLOCK_OPT_TABLE_SIZE "table_size"
#define BLOCK_OPT_PREALLOC "preallocation"
#define BLOCK_OPT_SUBFMT "subformat"
+#define BLOCK_OPT_TYPE "type"
typedef struct BdrvTrackedRequest BdrvTrackedRequest;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-01-31 23:04 ` Charles Arnold
@ 2012-01-31 23:15 ` Andreas Färber
2012-02-01 12:15 ` Kevin Wolf
1 sibling, 0 replies; 14+ messages in thread
From: Andreas Färber @ 2012-01-31 23:15 UTC (permalink / raw)
To: Charles Arnold; +Cc: Kevin Wolf, qemu-devel
Am 01.02.2012 00:04, schrieb Charles Arnold:
> The Virtual Hard Disk Image Format Specification allows for three
> types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
> currently only supports Dynamic disks. This patch adds support for
> the Fixed Disk format.
>
> Usage:
> Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
> Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output filename>
>
> While it is also allowed to specify '-o type=dynamic', the default disk type
> remains Dynamic and is what is used when the type is left unspecified.
>
> Signed-off-by: Charles Arnold <carnold@suse.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Thanks, looking good now AFAICS.
Andreas
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 89a5ee2..04db372 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -160,14 +160,25 @@ static int vpc_open(BlockDriverState *bs, int flags)
> struct vhd_dyndisk_header* dyndisk_header;
> uint8_t buf[HEADER_SIZE];
> uint32_t checksum;
> + int disk_type = VHD_DYNAMIC;
> int err = -1;
>
> if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
> goto fail;
>
> footer = (struct vhd_footer*) s->footer_buf;
> - if (strncmp(footer->creator, "conectix", 8))
> - goto fail;
> + if (strncmp(footer->creator, "conectix", 8)) {
> + int64_t offset = bdrv_getlength(bs->file);
> + /* If a fixed disk, the footer is found only at the end of the file */
> + if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
> + != HEADER_SIZE) {
> + goto fail;
> + }
> + if (strncmp(footer->creator, "conectix", 8)) {
> + goto fail;
> + }
> + disk_type = VHD_FIXED;
> + }
>
> checksum = be32_to_cpu(footer->checksum);
> footer->checksum = 0;
> @@ -186,6 +197,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
> goto fail;
> }
>
> + /* The footer is all that is needed for fixed disks */
> + if (disk_type == VHD_FIXED) {
> + /* The fixed disk format doesn't use footer->data_offset but it
> + should be initialized */
> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
> + return 0;
> + }
> +
> if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
> != HEADER_SIZE)
> goto fail;
> @@ -533,10 +552,10 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
> return 0;
> }
>
> -static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +static int vpc_create_dynamic_disk(const char *filename, int64_t total_size)
> {
> uint8_t buf[1024];
> - struct vhd_footer* footer = (struct vhd_footer*) buf;
> + struct vhd_footer* footer = (struct vhd_footer *) buf;
> struct vhd_dyndisk_header* dyndisk_header =
> (struct vhd_dyndisk_header*) buf;
> int fd, i;
> @@ -544,13 +563,9 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
> uint8_t heads = 0;
> uint8_t secs_per_cyl = 0;
> size_t block_size, num_bat_entries;
> - int64_t total_sectors = 0;
> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
> int ret = -EIO;
>
> - // Read out options
> - total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
> - BDRV_SECTOR_SIZE;
> -
> // Create the file
> fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> if (fd < 0)
> @@ -657,6 +672,101 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
> return ret;
> }
>
> +static int vpc_create_fixed_disk(const char *filename, int64_t total_size)
> +{
> + uint8_t buf[1024];
> + struct vhd_footer* footer = (struct vhd_footer *) buf;
> + int fd;
> + uint16_t cyls = 0;
> + uint8_t heads = 0;
> + uint8_t secs_per_cyl = 0;
> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
> + int ret = -EIO;
> +
> + /* Create the file */
> + fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> + if (fd < 0) {
> + return -EIO;
> + }
> +
> + /* Calculate matching total_size and geometry. */
> + if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
> + ret = -EFBIG;
> + goto fail;
> + }
> + total_sectors = (int64_t) cyls * heads * secs_per_cyl;
> +
> + /* Prepare the Hard Disk Footer */
> + memset(buf, 0, 1024);
> +
> + memcpy(footer->creator, "conectix", 8);
> + /* TODO Check if "qemu" creator_app is ok for VPC */
> + memcpy(footer->creator_app, "qemu", 4);
> + memcpy(footer->creator_os, "Wi2k", 4);
> +
> + footer->features = be32_to_cpu(0x02);
> + footer->version = be32_to_cpu(0x00010000);
> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
> + footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
> +
> + /* Version of Virtual PC 2007 */
> + footer->major = be16_to_cpu(0x0005);
> + footer->minor = be16_to_cpu(0x0003);
> + footer->orig_size = be64_to_cpu(total_size);
> + footer->size = be64_to_cpu(total_size);
> + footer->cyls = be16_to_cpu(cyls);
> + footer->heads = heads;
> + footer->secs_per_cyl = secs_per_cyl;
> +
> + footer->type = be32_to_cpu(VHD_FIXED);
> +
> + /* TODO uuid is missing */
> +
> + footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
> +
> + total_size += 512;
> + if (ftruncate(fd, total_size) != 0) {
> + ret = -errno;
> + goto fail;
> + }
> + if (lseek(fd, -512, SEEK_END) < 0) {
> + goto fail;
> + }
> + if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
> + goto fail;
> + }
> +
> + ret = 0;
> +
> + fail:
> + close(fd);
> + return ret;
> +}
> +
> +static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +{
> + int64_t total_size = 0;
> + QEMUOptionParameter *disk_type_param;
> + int ret = -EIO;
> +
> + /* Read out options */
> + total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
> +
> + disk_type_param = get_option_parameter(options, BLOCK_OPT_TYPE);
> + if (disk_type_param && disk_type_param->value.s) {
> + if (!strcmp(disk_type_param->value.s, "dynamic")) {
> + ret = vpc_create_dynamic_disk(filename, total_size);
> + } else if (!strcmp(disk_type_param->value.s, "fixed")) {
> + ret = vpc_create_fixed_disk(filename, total_size);
> + } else {
> + ret = -EINVAL;
> + }
> + } else {
> + ret = vpc_create_dynamic_disk(filename, total_size);
> + }
> + return ret;
> +}
> +
> static void vpc_close(BlockDriverState *bs)
> {
> BDRVVPCState *s = bs->opaque;
> @@ -675,6 +785,13 @@ static QEMUOptionParameter vpc_create_options[] = {
> .type = OPT_SIZE,
> .help = "Virtual disk size"
> },
> + {
> + .name = BLOCK_OPT_TYPE,
> + .type = OPT_STRING,
> + .help =
> + "Type of virtual hard disk format. Supported formats are "
> + "{dynamic (default) | fixed} "
> + },
> { NULL }
> };
>
> diff --git a/block_int.h b/block_int.h
> index 311bd2a..6d6cc96 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -50,6 +50,7 @@
> #define BLOCK_OPT_TABLE_SIZE "table_size"
> #define BLOCK_OPT_PREALLOC "preallocation"
> #define BLOCK_OPT_SUBFMT "subformat"
> +#define BLOCK_OPT_TYPE "type"
>
> typedef struct BdrvTrackedRequest BdrvTrackedRequest;
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-01-31 23:04 ` Charles Arnold
2012-01-31 23:15 ` Andreas Färber
@ 2012-02-01 12:15 ` Kevin Wolf
2012-02-01 16:51 ` Charles Arnold
1 sibling, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2012-02-01 12:15 UTC (permalink / raw)
To: Charles Arnold; +Cc: Andreas Färber, qemu-devel
Am 01.02.2012 00:04, schrieb Charles Arnold:
> Thanks Andreas,
>
> The 'TODO uuid is missing' comment in the patch is from the
> original sources (as well as many '//' comments). The vhd footer
> and header data structures contain a field for a UUID but no code
> was ever developed to generate one.
> The revised patch is below after running scripts/checkpatch.pl and
> fixing the 32 bit issues.
>
> - Charles
>
>
> The Virtual Hard Disk Image Format Specification allows for three
> types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
> currently only supports Dynamic disks. This patch adds support for
> the Fixed Disk format.
>
> Usage:
> Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
> Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output filename>
>
> While it is also allowed to specify '-o type=dynamic', the default disk type
> remains Dynamic and is what is used when the type is left unspecified.
>
> Signed-off-by: Charles Arnold <carnold@suse.com>
You have a lot of trailing whitespace in your patch, to the extent that
the patch is corrupted:
error: block/vpc.c : does not exist in
index
Please consider using git send-email.
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 89a5ee2..04db372 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -160,14 +160,25 @@ static int vpc_open(BlockDriverState *bs, int flags)
> struct vhd_dyndisk_header* dyndisk_header;
> uint8_t buf[HEADER_SIZE];
> uint32_t checksum;
> + int disk_type = VHD_DYNAMIC;
> int err = -1;
>
> if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
> goto fail;
>
> footer = (struct vhd_footer*) s->footer_buf;
> - if (strncmp(footer->creator, "conectix", 8))
> - goto fail;
> + if (strncmp(footer->creator, "conectix", 8)) {
> + int64_t offset = bdrv_getlength(bs->file);
bdrv_getlength can fail.
> + /* If a fixed disk, the footer is found only at the end of the file */
> + if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
> + != HEADER_SIZE) {
> + goto fail;
> + }
> + if (strncmp(footer->creator, "conectix", 8)) {
> + goto fail;
> + }
> + disk_type = VHD_FIXED;
> + }
>
> checksum = be32_to_cpu(footer->checksum);
> footer->checksum = 0;
> @@ -186,6 +197,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
> goto fail;
> }
>
> + /* The footer is all that is needed for fixed disks */
> + if (disk_type == VHD_FIXED) {
> + /* The fixed disk format doesn't use footer->data_offset but it
> + should be initialized */
> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
Why should it be changed? s->footer_buf is only used for updating the
footer, so you will change the value that is in the image file.
> + return 0;
This leaves most of BDRVVPCState uninitialised. I can't imagine how
bdrv_read/write could possibly work with an image in this state.
Something essential seems to be missing here.
> + }
> +
> if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
> != HEADER_SIZE)
> goto fail;
> @@ -533,10 +552,10 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
> return 0;
> }
>
> -static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +static int vpc_create_dynamic_disk(const char *filename, int64_t total_size)
> {
> uint8_t buf[1024];
> - struct vhd_footer* footer = (struct vhd_footer*) buf;
> + struct vhd_footer* footer = (struct vhd_footer *) buf;
Don't reformat existing code.
> struct vhd_dyndisk_header* dyndisk_header =
> (struct vhd_dyndisk_header*) buf;
> int fd, i;
> @@ -544,13 +563,9 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
> uint8_t heads = 0;
> uint8_t secs_per_cyl = 0;
> size_t block_size, num_bat_entries;
> - int64_t total_sectors = 0;
> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
> int ret = -EIO;
>
> - // Read out options
> - total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
> - BDRV_SECTOR_SIZE;
> -
> // Create the file
> fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> if (fd < 0)
> @@ -657,6 +672,101 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
> return ret;
> }
>
> +static int vpc_create_fixed_disk(const char *filename, int64_t total_size)
> +{
> + uint8_t buf[1024];
> + struct vhd_footer* footer = (struct vhd_footer *) buf;
> + int fd;
> + uint16_t cyls = 0;
> + uint8_t heads = 0;
> + uint8_t secs_per_cyl = 0;
> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
> + int ret = -EIO;
> +
> + /* Create the file */
> + fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> + if (fd < 0) {
> + return -EIO;
> + }
> +
> + /* Calculate matching total_size and geometry. */
> + if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
> + ret = -EFBIG;
> + goto fail;
> + }
> + total_sectors = (int64_t) cyls * heads * secs_per_cyl;
> +
> + /* Prepare the Hard Disk Footer */
> + memset(buf, 0, 1024);
> +
> + memcpy(footer->creator, "conectix", 8);
> + /* TODO Check if "qemu" creator_app is ok for VPC */
> + memcpy(footer->creator_app, "qemu", 4);
> + memcpy(footer->creator_os, "Wi2k", 4);
> +
> + footer->features = be32_to_cpu(0x02);
> + footer->version = be32_to_cpu(0x00010000);
> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
> + footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
> +
> + /* Version of Virtual PC 2007 */
> + footer->major = be16_to_cpu(0x0005);
> + footer->minor = be16_to_cpu(0x0003);
> + footer->orig_size = be64_to_cpu(total_size);
> + footer->size = be64_to_cpu(total_size);
> + footer->cyls = be16_to_cpu(cyls);
> + footer->heads = heads;
> + footer->secs_per_cyl = secs_per_cyl;
> +
> + footer->type = be32_to_cpu(VHD_FIXED);
> +
> + /* TODO uuid is missing */
> +
> + footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
> +
> + total_size += 512;
> + if (ftruncate(fd, total_size) != 0) {
> + ret = -errno;
> + goto fail;
> + }
> + if (lseek(fd, -512, SEEK_END) < 0) {
> + goto fail;
> + }
> + if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
> + goto fail;
> + }
> +
> + ret = 0;
> +
> + fail:
> + close(fd);
> + return ret;
> +}
This is a lot of duplication. You should be able to share some code with
vpc_create_dynamic.
> +
> +static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +{
> + int64_t total_size = 0;
> + QEMUOptionParameter *disk_type_param;
> + int ret = -EIO;
This value is unused, and -EIO wouldn't really make sense. Better leave
it uninitialised here and let the compiler warn if some case doesn't
change ret.
> +
> + /* Read out options */
> + total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
> +
> + disk_type_param = get_option_parameter(options, BLOCK_OPT_TYPE);
> + if (disk_type_param && disk_type_param->value.s) {
> + if (!strcmp(disk_type_param->value.s, "dynamic")) {
> + ret = vpc_create_dynamic_disk(filename, total_size);
> + } else if (!strcmp(disk_type_param->value.s, "fixed")) {
> + ret = vpc_create_fixed_disk(filename, total_size);
> + } else {
> + ret = -EINVAL;
> + }
> + } else {
> + ret = vpc_create_dynamic_disk(filename, total_size);
> + }
> + return ret;
> +}
> +
> static void vpc_close(BlockDriverState *bs)
> {
> BDRVVPCState *s = bs->opaque;
> @@ -675,6 +785,13 @@ static QEMUOptionParameter vpc_create_options[] = {
> .type = OPT_SIZE,
> .help = "Virtual disk size"
> },
> + {
> + .name = BLOCK_OPT_TYPE,
> + .type = OPT_STRING,
> + .help =
> + "Type of virtual hard disk format. Supported formats are "
> + "{dynamic (default) | fixed} "
> + },
> { NULL }
> };
>
> diff --git a/block_int.h b/block_int.h
> index 311bd2a..6d6cc96 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -50,6 +50,7 @@
> #define BLOCK_OPT_TABLE_SIZE "table_size"
> #define BLOCK_OPT_PREALLOC "preallocation"
> #define BLOCK_OPT_SUBFMT "subformat"
> +#define BLOCK_OPT_TYPE "type"
Looks like BLOCK_OPT_SUBFMT could be reused.
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-02-01 12:15 ` Kevin Wolf
@ 2012-02-01 16:51 ` Charles Arnold
2012-02-01 17:09 ` Stefan Weil
2012-02-02 8:58 ` Kevin Wolf
0 siblings, 2 replies; 14+ messages in thread
From: Charles Arnold @ 2012-02-01 16:51 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, afaerber
>>> On 2/1/2012 at 05:15 AM, in message <4F292CD0.20707@redhat.com>, Kevin Wolf
<kwolf@redhat.com> wrote:
> Am 01.02.2012 00:04, schrieb Charles Arnold:
>> Thanks Andreas,
>>
>> The 'TODO uuid is missing' comment in the patch is from the
>> original sources (as well as many '//' comments). The vhd footer
>> and header data structures contain a field for a UUID but no code
>> was ever developed to generate one.
>> The revised patch is below after running scripts/checkpatch.pl and
>> fixing the 32 bit issues.
>>
>> - Charles
>>
>>
>> The Virtual Hard Disk Image Format Specification allows for three
>> types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
>> currently only supports Dynamic disks. This patch adds support for
>> the Fixed Disk format.
>>
>> Usage:
>> Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
>> Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output
> filename>
>>
>> While it is also allowed to specify '-o type=dynamic', the default disk type
>> remains Dynamic and is what is used when the type is left unspecified.
>>
>> Signed-off-by: Charles Arnold <carnold@suse.com>
>
> You have a lot of trailing whitespace in your patch, to the extent that
> the patch is corrupted:
>
> error: block/vpc.c : does not exist in
> index
>
> Please consider using git send-email.
Sorry about that.
>
>>
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 89a5ee2..04db372 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -160,14 +160,25 @@ static int vpc_open(BlockDriverState *bs, int flags)
>> struct vhd_dyndisk_header* dyndisk_header;
>> uint8_t buf[HEADER_SIZE];
>> uint32_t checksum;
>> + int disk_type = VHD_DYNAMIC;
>> int err = -1;
>>
>> if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
>> goto fail;
>>
>> footer = (struct vhd_footer*) s->footer_buf;
>> - if (strncmp(footer->creator, "conectix", 8))
>> - goto fail;
>> + if (strncmp(footer->creator, "conectix", 8)) {
>> + int64_t offset = bdrv_getlength(bs->file);
>
> bdrv_getlength can fail.
Ok, I'll fix this.
>
>> + /* If a fixed disk, the footer is found only at the end of the file
> */
>> + if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
>> + != HEADER_SIZE) {
>
>> + goto fail;
>
>> + }
>
>> + if (strncmp(footer->creator, "conectix", 8)) {
>
>> + goto fail;
>
>> + }
>
>> + disk_type = VHD_FIXED;
>
>> + }
>
>>
>
>> checksum = be32_to_cpu(footer->checksum);
>
>> footer->checksum = 0;
>
>> @@ -186,6 +197,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
>
>> goto fail;
>
>> }
>
>>
>
>> + /* The footer is all that is needed for fixed disks */
>
>> + if (disk_type == VHD_FIXED) {
>
>> + /* The fixed disk format doesn't use footer->data_offset but it
>
>> + should be initialized */
>
>> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
>
>
> Why should it be changed? s->footer_buf is only used for updating the
> footer, so you will change the value that is in the image file.
The spec states the following about the data_offset field in the footer,
"This field is used for dynamic disks and differencing disks,
but not fixed disks. For fixed disks, this field should be set to 0xFFFFFFFF."
(Windows initializes all 8 bytes of the field)
>
>> + return 0;
>
> This leaves most of BDRVVPCState uninitialised. I can't imagine how
> bdrv_read/write could possibly work with an image in this state.
>
> Something essential seems to be missing here.
If vpc_open is opening a fixed disk, there is no dynamic disk header from
which to acquire information for filling out the BDRVVPCState structure.
However, you are right about the read/write code likely not working with
the structure left uninitialised. I'll look into what needs to be done here.
>
>> + }
>
>> +
>
>> if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
> HEADER_SIZE)
>> != HEADER_SIZE)
>
>> goto fail;
>
>> @@ -533,10 +552,10 @@ static int calculate_geometry(int64_t total_sectors,
> uint16_t* cyls,
>> return 0;
>
>> }
>
>>
>
>> -static int vpc_create(const char *filename, QEMUOptionParameter *options)
>
>> +static int vpc_create_dynamic_disk(const char *filename, int64_t
> total_size)
>> {
>
>> uint8_t buf[1024];
>
>> - struct vhd_footer* footer = (struct vhd_footer*) buf;
>
>> + struct vhd_footer* footer = (struct vhd_footer *) buf;
>
>
> Don't reformat existing code.
Even if scripts/checkpatch.pl complains?
What is the policy here if a patch contains changes that are within 3 lines
of existing code that doesn't follow the style guidelines?
>
>> struct vhd_dyndisk_header* dyndisk_header =
>
>> (struct vhd_dyndisk_header*) buf;
>
>> int fd, i;
>
>> @@ -544,13 +563,9 @@ static int vpc_create(const char *filename,
> QEMUOptionParameter *options)
>> uint8_t heads = 0;
>
>> uint8_t secs_per_cyl = 0;
>
>> size_t block_size, num_bat_entries;
>
>> - int64_t total_sectors = 0;
>
>> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
>
>> int ret = -EIO;
>
>>
>
>> - // Read out options
>
>> - total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
>
>> - BDRV_SECTOR_SIZE;
>
>> -
>
>> // Create the file
>
>> fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
>
>> if (fd < 0)
>
>> @@ -657,6 +672,101 @@ static int vpc_create(const char *filename,
> QEMUOptionParameter *options)
>> return ret;
>
>> }
>
>>
>
>> +static int vpc_create_fixed_disk(const char *filename, int64_t total_size)
>
>> +{
>
>> + uint8_t buf[1024];
>
>> + struct vhd_footer* footer = (struct vhd_footer *) buf;
>
>> + int fd;
>
>> + uint16_t cyls = 0;
>
>> + uint8_t heads = 0;
>
>> + uint8_t secs_per_cyl = 0;
>
>> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
>
>> + int ret = -EIO;
>
>> +
>
>> + /* Create the file */
>
>> + fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
>
>> + if (fd < 0) {
>
>> + return -EIO;
>
>> + }
>
>> +
>
>> + /* Calculate matching total_size and geometry. */
>
>> + if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
>
>> + ret = -EFBIG;
>
>> + goto fail;
>
>> + }
>
>> + total_sectors = (int64_t) cyls * heads * secs_per_cyl;
>
>> +
>
>> + /* Prepare the Hard Disk Footer */
>
>> + memset(buf, 0, 1024);
>
>> +
>
>> + memcpy(footer->creator, "conectix", 8);
>
>> + /* TODO Check if "qemu" creator_app is ok for VPC */
>
>> + memcpy(footer->creator_app, "qemu", 4);
>
>> + memcpy(footer->creator_os, "Wi2k", 4);
>
>> +
>
>> + footer->features = be32_to_cpu(0x02);
>
>> + footer->version = be32_to_cpu(0x00010000);
>
>> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
>
>> + footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
>
>> +
>
>> + /* Version of Virtual PC 2007 */
>
>> + footer->major = be16_to_cpu(0x0005);
>
>> + footer->minor = be16_to_cpu(0x0003);
>
>> + footer->orig_size = be64_to_cpu(total_size);
>
>> + footer->size = be64_to_cpu(total_size);
>
>> + footer->cyls = be16_to_cpu(cyls);
>
>> + footer->heads = heads;
>
>> + footer->secs_per_cyl = secs_per_cyl;
>
>> +
>
>> + footer->type = be32_to_cpu(VHD_FIXED);
>
>> +
>
>> + /* TODO uuid is missing */
>
>> +
>
>> + footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
>
>> +
>
>> + total_size += 512;
>
>> + if (ftruncate(fd, total_size) != 0) {
>
>> + ret = -errno;
>
>> + goto fail;
>
>> + }
>
>> + if (lseek(fd, -512, SEEK_END) < 0) {
>
>> + goto fail;
>
>> + }
>
>> + if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
>> + goto fail;
>> + }
>> +
>> + ret = 0;
>> +
>> + fail:
>> + close(fd);
>> + return ret;
>> +}
>
> This is a lot of duplication. You should be able to share some code with
> vpc_create_dynamic.
Yes, I noticed that. My original work just created a flag that was used in the original
vpc_create without splitting out two additional functions.
Perhaps I should have kept my original approach.
>
>> +
>> +static int vpc_create(const char *filename, QEMUOptionParameter *options)
>> +{
>> + int64_t total_size = 0;
>> + QEMUOptionParameter *disk_type_param;
>> + int ret = -EIO;
>
> This value is unused, and -EIO wouldn't really make sense. Better leave
> it uninitialised here and let the compiler warn if some case doesn't
> change ret.
Ok.
>
>> +
>> + /* Read out options */
>> + total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
>> +
>> + disk_type_param = get_option_parameter(options, BLOCK_OPT_TYPE);
>> + if (disk_type_param && disk_type_param->value.s) {
>> + if (!strcmp(disk_type_param->value.s, "dynamic")) {
>> + ret = vpc_create_dynamic_disk(filename, total_size);
>> + } else if (!strcmp(disk_type_param->value.s, "fixed")) {
>> + ret = vpc_create_fixed_disk(filename, total_size);
>> + } else {
>> + ret = -EINVAL;
>> + }
>> + } else {
>> + ret = vpc_create_dynamic_disk(filename, total_size);
>> + }
>> + return ret;
>> +}
>> +
>> static void vpc_close(BlockDriverState *bs)
>> {
>> BDRVVPCState *s = bs->opaque;
>> @@ -675,6 +785,13 @@ static QEMUOptionParameter vpc_create_options[] = {
>> .type = OPT_SIZE,
>> .help = "Virtual disk size"
>> },
>> + {
>> + .name = BLOCK_OPT_TYPE,
>> + .type = OPT_STRING,
>> + .help =
>> + "Type of virtual hard disk format. Supported formats are "
>> + "{dynamic (default) | fixed} "
>> + },
>> { NULL }
>> };
>>
>> diff --git a/block_int.h b/block_int.h
>> index 311bd2a..6d6cc96 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -50,6 +50,7 @@
>> #define BLOCK_OPT_TABLE_SIZE "table_size"
>> #define BLOCK_OPT_PREALLOC "preallocation"
>> #define BLOCK_OPT_SUBFMT "subformat"
>> +#define BLOCK_OPT_TYPE "type"
>
> Looks like BLOCK_OPT_SUBFMT could be reused.
Sure, if that is the preferred option.
>
> Kevin
Thanks for taking the time to review the patch. I'll rework it and send it out again.
- Charles
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-02-01 16:51 ` Charles Arnold
@ 2012-02-01 17:09 ` Stefan Weil
2012-02-02 8:58 ` Kevin Wolf
1 sibling, 0 replies; 14+ messages in thread
From: Stefan Weil @ 2012-02-01 17:09 UTC (permalink / raw)
To: Charles Arnold; +Cc: Kevin Wolf, qemu-devel, afaerber
Am 01.02.2012 17:51, schrieb Charles Arnold:
>>>> On 2/1/2012 at 05:15 AM, in message<4F292CD0.20707@redhat.com>, Kevin Wolf
>>>>
> <kwolf@redhat.com> wrote:
>
>> Am 01.02.2012 00:04, schrieb Charles Arnold:
>>
>>> Thanks Andreas,
>>>
>>> The 'TODO uuid is missing' comment in the patch is from the
>>> original sources (as well as many '//' comments). The vhd footer
>>> and header data structures contain a field for a UUID but no code
>>> was ever developed to generate one.
>>> The revised patch is below after running scripts/checkpatch.pl and
>>> fixing the 32 bit issues.
>>>
>>> - Charles
>>>
>>>
>>> The Virtual Hard Disk Image Format Specification allows for three
>>> types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
>>> currently only supports Dynamic disks. This patch adds support for
>>> the Fixed Disk format.
>>>
>>> Usage:
>>> Example 1: qemu-img create -f vpc -o type=fixed<filename> [size]
>>> Example 2: qemu-img convert -O vpc -o type=fixed<input filename> <output
>>>
>> filename>
>>
>>> While it is also allowed to specify '-o type=dynamic', the default disk type
>>> remains Dynamic and is what is used when the type is left unspecified.
>>>
>>> Signed-off-by: Charles Arnold<carnold@suse.com>
>>>
>> You have a lot of trailing whitespace in your patch, to the extent that
>> the patch is corrupted:
>>
>> error: block/vpc.c : does not exist in
>> index
>>
>> Please consider using git send-email.
>>
> Sorry about that.
>
>
>>
>>> diff --git a/block/vpc.c b/block/vpc.c
>>> index 89a5ee2..04db372 100644
>>> --- a/block/vpc.c
>>> +++ b/block/vpc.c
>>> @@ -160,14 +160,25 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>> struct vhd_dyndisk_header* dyndisk_header;
>>> uint8_t buf[HEADER_SIZE];
>>> uint32_t checksum;
>>> + int disk_type = VHD_DYNAMIC;
>>> int err = -1;
>>>
>>> if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
>>> goto fail;
>>>
>>> footer = (struct vhd_footer*) s->footer_buf;
>>> - if (strncmp(footer->creator, "conectix", 8))
>>> - goto fail;
>>> + if (strncmp(footer->creator, "conectix", 8)) {
>>> + int64_t offset = bdrv_getlength(bs->file);
>>>
>> bdrv_getlength can fail.
>>
> Ok, I'll fix this.
>
>
>>
>>> + /* If a fixed disk, the footer is found only at the end of the file
>>>
>> */
>>
>>> + if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
>>> + != HEADER_SIZE) {
>>>
>>
>>
>>> + goto fail;
>>>
>>
>>
>>> + }
>>>
>>
>>
>>> + if (strncmp(footer->creator, "conectix", 8)) {
>>>
>>
>>
>>> + goto fail;
>>>
>>
>>
>>> + }
>>>
>>
>>
>>> + disk_type = VHD_FIXED;
>>>
>>
>>
>>> + }
>>>
>>
>>
>>>
>>>
>>
>>
>>> checksum = be32_to_cpu(footer->checksum);
>>>
>>
>>
>>> footer->checksum = 0;
>>>
>>
>>
>>> @@ -186,6 +197,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>>
>>
>>
>>> goto fail;
>>>
>>
>>
>>> }
>>>
>>
>>
>>>
>>>
>>
>>
>>> + /* The footer is all that is needed for fixed disks */
>>>
>>
>>
>>> + if (disk_type == VHD_FIXED) {
>>>
>>
>>
>>> + /* The fixed disk format doesn't use footer->data_offset but it
>>>
>>
>>
>>> + should be initialized */
>>>
>>
>>
>>> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
>>>
>>
>>
>> Why should it be changed? s->footer_buf is only used for updating the
>> footer, so you will change the value that is in the image file.
>>
> The spec states the following about the data_offset field in the footer,
> "This field is used for dynamic disks and differencing disks,
> but not fixed disks. For fixed disks, this field should be set to 0xFFFFFFFF."
> (Windows initializes all 8 bytes of the field)
>
>
>>
>>> + return 0;
>>>
>> This leaves most of BDRVVPCState uninitialised. I can't imagine how
>> bdrv_read/write could possibly work with an image in this state.
>>
>> Something essential seems to be missing here.
>>
> If vpc_open is opening a fixed disk, there is no dynamic disk header from
> which to acquire information for filling out the BDRVVPCState structure.
> However, you are right about the read/write code likely not working with
> the structure left uninitialised. I'll look into what needs to be done here.
>
>
>>
>>> + }
>>>
>>
>>
>>> +
>>>
>>
>>
>>> if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
>>>
>> HEADER_SIZE)
>>
>>> != HEADER_SIZE)
>>>
>>
>>
>>> goto fail;
>>>
>>
>>
>>> @@ -533,10 +552,10 @@ static int calculate_geometry(int64_t total_sectors,
>>>
>> uint16_t* cyls,
>>
>>> return 0;
>>>
>>
>>
>>> }
>>>
>>
>>
>>>
>>>
>>
>>
>>> -static int vpc_create(const char *filename, QEMUOptionParameter *options)
>>>
>>
>>
>>> +static int vpc_create_dynamic_disk(const char *filename, int64_t
>>>
>> total_size)
>>
>>> {
>>>
>>
>>
>>> uint8_t buf[1024];
>>>
>>
>>
>>> - struct vhd_footer* footer = (struct vhd_footer*) buf;
>>>
>>
>>
>>> + struct vhd_footer* footer = (struct vhd_footer *) buf;
>>>
>>
>>
>> Don't reformat existing code.
>>
>
> Even if scripts/checkpatch.pl complains?
> What is the policy here if a patch contains changes that are within 3 lines
> of existing code that doesn't follow the style guidelines?
>
Your patch should pass checkpatch.pl (I think it did not because
of several violations of the QEMU coding style).
Unmodified existing code should not raise warnings from checkpatch.pl.
Regards,
Stefan Weil
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-02-01 16:51 ` Charles Arnold
2012-02-01 17:09 ` Stefan Weil
@ 2012-02-02 8:58 ` Kevin Wolf
2012-02-03 0:16 ` Charles Arnold
1 sibling, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2012-02-02 8:58 UTC (permalink / raw)
To: Charles Arnold; +Cc: qemu-devel, afaerber
Am 01.02.2012 17:51, schrieb Charles Arnold:
>>>> On 2/1/2012 at 05:15 AM, in message <4F292CD0.20707@redhat.com>, Kevin Wolf
> <kwolf@redhat.com> wrote:
>> Am 01.02.2012 00:04, schrieb Charles Arnold:
>>> Thanks Andreas,
>>>
>>> The 'TODO uuid is missing' comment in the patch is from the
>>> original sources (as well as many '//' comments). The vhd footer
>>> and header data structures contain a field for a UUID but no code
>>> was ever developed to generate one.
>>> The revised patch is below after running scripts/checkpatch.pl and
>>> fixing the 32 bit issues.
>>>
>>> - Charles
>>>
>>>
>>> The Virtual Hard Disk Image Format Specification allows for three
>>> types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
>>> currently only supports Dynamic disks. This patch adds support for
>>> the Fixed Disk format.
>>>
>>> Usage:
>>> Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
>>> Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output
>> filename>
>>>
>>> While it is also allowed to specify '-o type=dynamic', the default disk type
>>> remains Dynamic and is what is used when the type is left unspecified.
>>>
>>> Signed-off-by: Charles Arnold <carnold@suse.com>
>>> @@ -186,6 +197,14 @@ static int vpc_open(BlockDriverState *bs, int flags)
>>
>>> goto fail;
>>
>>> }
>>
>>>
>>
>>> + /* The footer is all that is needed for fixed disks */
>>
>>> + if (disk_type == VHD_FIXED) {
>>
>>> + /* The fixed disk format doesn't use footer->data_offset but it
>>
>>> + should be initialized */
>>
>>> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
>>
>>
>> Why should it be changed? s->footer_buf is only used for updating the
>> footer, so you will change the value that is in the image file.
>
> The spec states the following about the data_offset field in the footer,
> "This field is used for dynamic disks and differencing disks,
> but not fixed disks. For fixed disks, this field should be set to 0xFFFFFFFF."
> (Windows initializes all 8 bytes of the field)
Which is relevant when creating images (there we do set data_offset to
0xFFFFFFFFFFFFFFFF), but not when opening images. If anything, you could
check if the value is set right and return an error otherwise.
>>> + return 0;
>>
>> This leaves most of BDRVVPCState uninitialised. I can't imagine how
>> bdrv_read/write could possibly work with an image in this state.
>>
>> Something essential seems to be missing here.
>
> If vpc_open is opening a fixed disk, there is no dynamic disk header from
> which to acquire information for filling out the BDRVVPCState structure.
> However, you are right about the read/write code likely not working with
> the structure left uninitialised. I'll look into what needs to be done here.
The easiest way is probably to set a field in BDRVVPCState that
remembers the image type, and have two versions of get_sector_offset().
Dynamic images would use the existing one, and fixed a new trivial one
that checks if the sector_num is within the image and returns 512 *
sector_num.
alloc_block() needs to fail for fixed images. In fact you could even
assert() that it doesn't happen.
>>> + }
>>
>>> +
>>
>>> if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
>> HEADER_SIZE)
>>> != HEADER_SIZE)
>>
>>> goto fail;
>>
>>> @@ -533,10 +552,10 @@ static int calculate_geometry(int64_t total_sectors,
>> uint16_t* cyls,
>>> return 0;
>>
>>> }
>>
>>>
>>
>>> -static int vpc_create(const char *filename, QEMUOptionParameter *options)
>>
>>> +static int vpc_create_dynamic_disk(const char *filename, int64_t
>> total_size)
>>> {
>>
>>> uint8_t buf[1024];
>>
>>> - struct vhd_footer* footer = (struct vhd_footer*) buf;
>>
>>> + struct vhd_footer* footer = (struct vhd_footer *) buf;
>>
>>
>> Don't reformat existing code.
>
>
> Even if scripts/checkpatch.pl complains?
> What is the policy here if a patch contains changes that are within 3 lines
> of existing code that doesn't follow the style guidelines?
checkpatch.pl should only complain about newly added lines.
>>> +static int vpc_create_fixed_disk(const char *filename, int64_t total_size)
>>
>>> +{
>>
>>> + uint8_t buf[1024];
>>
>>> + struct vhd_footer* footer = (struct vhd_footer *) buf;
>>
>>> + int fd;
>>
>>> + uint16_t cyls = 0;
>>
>>> + uint8_t heads = 0;
>>
>>> + uint8_t secs_per_cyl = 0;
>>
>>> + int64_t total_sectors = total_size / BDRV_SECTOR_SIZE;
>>
>>> + int ret = -EIO;
>>
>>> +
>>
>>> + /* Create the file */
>>
>>> + fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
>>
>>> + if (fd < 0) {
>>
>>> + return -EIO;
>>
>>> + }
>>
>>> +
>>
>>> + /* Calculate matching total_size and geometry. */
>>
>>> + if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
>>
>>> + ret = -EFBIG;
>>
>>> + goto fail;
>>
>>> + }
>>
>>> + total_sectors = (int64_t) cyls * heads * secs_per_cyl;
>>
>>> +
>>
>>> + /* Prepare the Hard Disk Footer */
>>
>>> + memset(buf, 0, 1024);
>>
>>> +
>>
>>> + memcpy(footer->creator, "conectix", 8);
>>
>>> + /* TODO Check if "qemu" creator_app is ok for VPC */
>>
>>> + memcpy(footer->creator_app, "qemu", 4);
>>
>>> + memcpy(footer->creator_os, "Wi2k", 4);
>>
>>> +
>>
>>> + footer->features = be32_to_cpu(0x02);
>>
>>> + footer->version = be32_to_cpu(0x00010000);
>>
>>> + footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
>>
>>> + footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
>>
>>> +
>>
>>> + /* Version of Virtual PC 2007 */
>>
>>> + footer->major = be16_to_cpu(0x0005);
>>
>>> + footer->minor = be16_to_cpu(0x0003);
>>
>>> + footer->orig_size = be64_to_cpu(total_size);
>>
>>> + footer->size = be64_to_cpu(total_size);
>>
>>> + footer->cyls = be16_to_cpu(cyls);
>>
>>> + footer->heads = heads;
>>
>>> + footer->secs_per_cyl = secs_per_cyl;
>>
>>> +
>>
>>> + footer->type = be32_to_cpu(VHD_FIXED);
>>
>>> +
>>
>>> + /* TODO uuid is missing */
>>
>>> +
>>
>>> + footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
>>
>>> +
>>
>>> + total_size += 512;
>>
>>> + if (ftruncate(fd, total_size) != 0) {
>>
>>> + ret = -errno;
>>
>>> + goto fail;
>>
>>> + }
>>
>>> + if (lseek(fd, -512, SEEK_END) < 0) {
>>
>>> + goto fail;
>>
>>> + }
>>
>>> + if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
>>> + goto fail;
>>> + }
>>> +
>>> + ret = 0;
>>> +
>>> + fail:
>>> + close(fd);
>>> + return ret;
>>> +}
>>
>> This is a lot of duplication. You should be able to share some code with
>> vpc_create_dynamic.
>
> Yes, I noticed that. My original work just created a flag that was used in the original
> vpc_create without splitting out two additional functions.
> Perhaps I should have kept my original approach.
You could keep the first part (calculation of the geometry, filling most
of the footer, opening the file) common and call two separate functions
only for the rest. This way most of the duplication should be avoided.
>>> diff --git a/block_int.h b/block_int.h
>>> index 311bd2a..6d6cc96 100644
>>> --- a/block_int.h
>>> +++ b/block_int.h
>>> @@ -50,6 +50,7 @@
>>> #define BLOCK_OPT_TABLE_SIZE "table_size"
>>> #define BLOCK_OPT_PREALLOC "preallocation"
>>> #define BLOCK_OPT_SUBFMT "subformat"
>>> +#define BLOCK_OPT_TYPE "type"
>>
>> Looks like BLOCK_OPT_SUBFMT could be reused.
>
> Sure, if that is the preferred option.
I think it helps keeping a consistent interface across image formats,
though I won't insist on it if there are good reasons to do otherwise.
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-02-02 8:58 ` Kevin Wolf
@ 2012-02-03 0:16 ` Charles Arnold
2012-02-06 15:46 ` Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Charles Arnold @ 2012-02-03 0:16 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, afaerber
Next version of the patch with fixes, cleanups, and suggestions.
- Charles
The Virtual Hard Disk Image Format Specification allows for three
types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
currently only supports Dynamic disks. This patch adds support for
the Fixed Disk format.
Usage:
Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output filename>
While it is also allowed to specify '-o type=dynamic', the default disk type
remains Dynamic and is what is used when the type is left unspecified.
Signed-off-by: Charles Arnold <carnold@suse.com>
diff --git a/block/vpc.c b/block/vpc.c
index 89a5ee2..d9a1c44 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -161,13 +161,27 @@ static int vpc_open(BlockDriverState *bs, int flags)
uint8_t buf[HEADER_SIZE];
uint32_t checksum;
int err = -1;
+ int disk_type = VHD_DYNAMIC;
if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
goto fail;
footer = (struct vhd_footer*) s->footer_buf;
- if (strncmp(footer->creator, "conectix", 8))
- goto fail;
+ if (strncmp(footer->creator, "conectix", 8)) {
+ int64_t offset = bdrv_getlength(bs->file);
+ if (offset < HEADER_SIZE) {
+ goto fail;
+ }
+ /* If a fixed disk, the footer is found only at the end of the file */
+ if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
+ != HEADER_SIZE) {
+ goto fail;
+ }
+ if (strncmp(footer->creator, "conectix", 8)) {
+ goto fail;
+ }
+ disk_type = VHD_FIXED;
+ }
checksum = be32_to_cpu(footer->checksum);
footer->checksum = 0;
@@ -186,49 +200,54 @@ static int vpc_open(BlockDriverState *bs, int flags)
goto fail;
}
- if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
- != HEADER_SIZE)
- goto fail;
-
- dyndisk_header = (struct vhd_dyndisk_header*) buf;
+ if (disk_type == VHD_DYNAMIC) {
+ if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
+ HEADER_SIZE) != HEADER_SIZE) {
+ goto fail;
+ }
- if (strncmp(dyndisk_header->magic, "cxsparse", 8))
- goto fail;
+ dyndisk_header = (struct vhd_dyndisk_header *) buf;
+ if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
+ goto fail;
+ }
- s->block_size = be32_to_cpu(dyndisk_header->block_size);
- s->bitmap_size = ((s->block_size / (8 * 512)) + 511) & ~511;
+ s->block_size = be32_to_cpu(dyndisk_header->block_size);
+ s->bitmap_size = ((s->block_size / (8 * 512)) + 511) & ~511;
- s->max_table_entries = be32_to_cpu(dyndisk_header->max_table_entries);
- s->pagetable = g_malloc(s->max_table_entries * 4);
+ s->max_table_entries = be32_to_cpu(dyndisk_header->max_table_entries);
+ s->pagetable = g_malloc(s->max_table_entries * 4);
- s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
- if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
- s->max_table_entries * 4) != s->max_table_entries * 4)
- goto fail;
+ s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
+ if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
+ s->max_table_entries * 4) != s->max_table_entries * 4) {
+ goto fail;
+ }
- s->free_data_block_offset =
- (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511;
+ s->free_data_block_offset =
+ (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511;
- for (i = 0; i < s->max_table_entries; i++) {
- be32_to_cpus(&s->pagetable[i]);
- if (s->pagetable[i] != 0xFFFFFFFF) {
- int64_t next = (512 * (int64_t) s->pagetable[i]) +
- s->bitmap_size + s->block_size;
+ for (i = 0; i < s->max_table_entries; i++) {
+ be32_to_cpus(&s->pagetable[i]);
+ if (s->pagetable[i] != 0xFFFFFFFF) {
+ int64_t next = (512 * (int64_t) s->pagetable[i]) +
+ s->bitmap_size + s->block_size;
- if (next> s->free_data_block_offset)
- s->free_data_block_offset = next;
+ if (next > s->free_data_block_offset) {
+ s->free_data_block_offset = next;
+ }
+ }
}
- }
- s->last_bitmap_offset = (int64_t) -1;
+ s->last_bitmap_offset = (int64_t) -1;
#ifdef CACHE
- s->pageentry_u8 = g_malloc(512);
- s->pageentry_u32 = s->pageentry_u8;
- s->pageentry_u16 = s->pageentry_u8;
- s->last_pagetable = -1;
+ s->pageentry_u8 = g_malloc(512);
+ s->pageentry_u32 = s->pageentry_u8;
+ s->pageentry_u16 = s->pageentry_u8;
+ s->last_pagetable = -1;
#endif
+ }
qemu_co_mutex_init(&s->lock);
@@ -395,7 +414,11 @@ static int vpc_read(BlockDriverState *bs, int64_t sector_num,
int ret;
int64_t offset;
int64_t sectors, sectors_per_block;
+ struct vhd_footer *footer = (struct vhd_footer *) s->footer_buf;
+ if (cpu_to_be32(footer->type) == VHD_FIXED) {
+ return bdrv_read(bs->file, sector_num, buf, nb_sectors);
+ }
while (nb_sectors > 0) {
offset = get_sector_offset(bs, sector_num, 0);
@@ -440,7 +463,11 @@ static int vpc_write(BlockDriverState *bs, int64_t sector_num,
int64_t offset;
int64_t sectors, sectors_per_block;
int ret;
+ struct vhd_footer *footer = (struct vhd_footer *) s->footer_buf;
+ if (cpu_to_be32(footer->type) == VHD_FIXED) {
+ return bdrv_write(bs->file, sector_num, buf, nb_sectors);
+ }
while (nb_sectors > 0) {
offset = get_sector_offset(bs, sector_num, 1);
@@ -533,70 +560,14 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
return 0;
}
-static int vpc_create(const char *filename, QEMUOptionParameter *options)
+static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
{
- uint8_t buf[1024];
- struct vhd_footer* footer = (struct vhd_footer*) buf;
struct vhd_dyndisk_header* dyndisk_header =
(struct vhd_dyndisk_header*) buf;
- int fd, i;
- uint16_t cyls = 0;
- uint8_t heads = 0;
- uint8_t secs_per_cyl = 0;
size_t block_size, num_bat_entries;
- int64_t total_sectors = 0;
+ int i;
int ret = -EIO;
- // Read out options
- total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
- BDRV_SECTOR_SIZE;
-
- // Create the file
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
- if (fd < 0)
- return -EIO;
-
- /* Calculate matching total_size and geometry. Increase the number of
- sectors requested until we get enough (or fail). */
- 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 fail;
- }
- }
- total_sectors = (int64_t) cyls * heads * secs_per_cyl;
-
- // Prepare the Hard Disk Footer
- memset(buf, 0, 1024);
-
- memcpy(footer->creator, "conectix", 8);
- // TODO Check if "qemu" creator_app is ok for VPC
- memcpy(footer->creator_app, "qemu", 4);
- memcpy(footer->creator_os, "Wi2k", 4);
-
- footer->features = be32_to_cpu(0x02);
- footer->version = be32_to_cpu(0x00010000);
- footer->data_offset = be64_to_cpu(HEADER_SIZE);
- footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
-
- // Version of Virtual PC 2007
- footer->major = be16_to_cpu(0x0005);
- footer->minor =be16_to_cpu(0x0003);
-
- footer->orig_size = be64_to_cpu(total_sectors * 512);
- footer->size = be64_to_cpu(total_sectors * 512);
-
- footer->cyls = be16_to_cpu(cyls);
- footer->heads = heads;
- footer->secs_per_cyl = secs_per_cyl;
-
- footer->type = be32_to_cpu(VHD_DYNAMIC);
-
- // TODO uuid is missing
-
- footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
-
// Write the footer (twice: at the beginning and at the end)
block_size = 0x200000;
num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
@@ -624,7 +595,6 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
}
}
-
// Prepare the Dynamic Disk Header
memset(buf, 0, 1024);
@@ -653,6 +623,130 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
ret = 0;
fail:
+ return ret;
+}
+
+static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
+{
+ int ret = -EIO;
+
+ /* Add footer to total size */
+ total_size += 512;
+ if (ftruncate(fd, total_size) != 0) {
+ ret = -errno;
+ goto fail;
+ }
+ if (lseek(fd, -512, SEEK_END) < 0) {
+ goto fail;
+ }
+ if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
+ goto fail;
+ }
+
+ ret = 0;
+
+ fail:
+ return ret;
+}
+
+static int vpc_create(const char *filename, QEMUOptionParameter *options)
+{
+ uint8_t buf[1024];
+ struct vhd_footer *footer = (struct vhd_footer *) buf;
+ QEMUOptionParameter *disk_type_param;
+ int fd, i;
+ uint16_t cyls = 0;
+ uint8_t heads = 0;
+ uint8_t secs_per_cyl = 0;
+ int64_t total_sectors;
+ int64_t total_size;
+ int disk_type;
+ int ret = -EIO;
+
+ /* Read out options */
+ total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
+
+ disk_type_param = get_option_parameter(options, BLOCK_OPT_SUBFMT);
+ if (disk_type_param && disk_type_param->value.s) {
+ if (!strcmp(disk_type_param->value.s, "dynamic")) {
+ disk_type = VHD_DYNAMIC;
+ } else if (!strcmp(disk_type_param->value.s, "fixed")) {
+ disk_type = VHD_FIXED;
+ } else {
+ return -EINVAL;
+ }
+ } else {
+ disk_type = VHD_DYNAMIC;
+ }
+
+ /* Create the file */
+ fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
+ if (fd < 0) {
+ return -EIO;
+ }
+
+ total_sectors = total_size / BDRV_SECTOR_SIZE;
+ if (disk_type == VHD_DYNAMIC) {
+ /* Calculate matching total_size and geometry. Increase the number of
+ sectors requested until we get enough (or fail). */
+ for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl;
+ i++) {
+ if (calculate_geometry(total_sectors + i,
+ &cyls, &heads, &secs_per_cyl)) {
+ goto fail;
+ }
+ }
+ } else {
+ if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
+ goto fail;
+ }
+ }
+ total_sectors = (int64_t) cyls * heads * secs_per_cyl;
+
+ /* Prepare the Hard Disk Footer */
+ memset(buf, 0, 1024);
+
+ memcpy(footer->creator, "conectix", 8);
+ /* TODO Check if "qemu" creator_app is ok for VPC */
+ memcpy(footer->creator_app, "qemu", 4);
+ memcpy(footer->creator_os, "Wi2k", 4);
+
+ footer->features = be32_to_cpu(0x02);
+ footer->version = be32_to_cpu(0x00010000);
+ if (disk_type == VHD_DYNAMIC) {
+ footer->data_offset = be64_to_cpu(HEADER_SIZE);
+ } else {
+ footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
+ }
+ footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
+
+ /* Version of Virtual PC 2007 */
+ footer->major = be16_to_cpu(0x0005);
+ footer->minor = be16_to_cpu(0x0003);
+ if (disk_type == VHD_DYNAMIC) {
+ footer->orig_size = be64_to_cpu(total_sectors * 512);
+ footer->size = be64_to_cpu(total_sectors * 512);
+ } else {
+ footer->orig_size = be64_to_cpu(total_size);
+ footer->size = be64_to_cpu(total_size);
+ }
+ footer->cyls = be16_to_cpu(cyls);
+ footer->heads = heads;
+ footer->secs_per_cyl = secs_per_cyl;
+
+ footer->type = be32_to_cpu(disk_type);
+
+ /* TODO uuid is missing */
+
+ footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
+
+ if (disk_type == VHD_DYNAMIC) {
+ ret = create_dynamic_disk(fd, buf, total_sectors);
+ } else {
+ ret = create_fixed_disk(fd, buf, total_size);
+ }
+
+ fail:
close(fd);
return ret;
}
@@ -675,6 +769,13 @@ static QEMUOptionParameter vpc_create_options[] = {
.type = OPT_SIZE,
.help = "Virtual disk size"
},
+ {
+ .name = BLOCK_OPT_SUBFMT,
+ .type = OPT_STRING,
+ .help =
+ "Type of virtual hard disk format. Supported formats are "
+ "{dynamic (default) | fixed} "
+ },
{ NULL }
};
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-02-03 0:16 ` Charles Arnold
@ 2012-02-06 15:46 ` Kevin Wolf
2012-02-06 16:22 ` Charles Arnold
0 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2012-02-06 15:46 UTC (permalink / raw)
To: Charles Arnold; +Cc: qemu-devel, afaerber
Am 03.02.2012 01:16, schrieb Charles Arnold:
> Next version of the patch with fixes, cleanups, and suggestions.
> - Charles
>
>
> The Virtual Hard Disk Image Format Specification allows for three
> types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
> currently only supports Dynamic disks. This patch adds support for
> the Fixed Disk format.
>
> Usage:
> Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
> Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output filename>
>
> While it is also allowed to specify '-o type=dynamic', the default disk type
> remains Dynamic and is what is used when the type is left unspecified.
>
> Signed-off-by: Charles Arnold <carnold@suse.com>
>
> diff --git a/block/vpc.c b/block/vpc.c
> index 89a5ee2..d9a1c44 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -161,13 +161,27 @@ static int vpc_open(BlockDriverState *bs, int flags)
> uint8_t buf[HEADER_SIZE];
> uint32_t checksum;
> int err = -1;
> + int disk_type = VHD_DYNAMIC;
>
> if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
> goto fail;
>
> footer = (struct vhd_footer*) s->footer_buf;
> - if (strncmp(footer->creator, "conectix", 8))
> - goto fail;
> + if (strncmp(footer->creator, "conectix", 8)) {
> + int64_t offset = bdrv_getlength(bs->file);
> + if (offset < HEADER_SIZE) {
> + goto fail;
> + }
> + /* If a fixed disk, the footer is found only at the end of the file */
> + if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
> + != HEADER_SIZE) {
> + goto fail;
> + }
> + if (strncmp(footer->creator, "conectix", 8)) {
> + goto fail;
> + }
> + disk_type = VHD_FIXED;
> + }
>
> checksum = be32_to_cpu(footer->checksum);
> footer->checksum = 0;
> @@ -186,49 +200,54 @@ static int vpc_open(BlockDriverState *bs, int flags)
> goto fail;
> }
>
> - if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
> - != HEADER_SIZE)
> - goto fail;
> -
> - dyndisk_header = (struct vhd_dyndisk_header*) buf;
> + if (disk_type == VHD_DYNAMIC) {
> + if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
> + HEADER_SIZE) != HEADER_SIZE) {
> + goto fail;
> + }
>
> - if (strncmp(dyndisk_header->magic, "cxsparse", 8))
> - goto fail;
> + dyndisk_header = (struct vhd_dyndisk_header *) buf;
>
> + if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
> + goto fail;
> + }
>
> - s->block_size = be32_to_cpu(dyndisk_header->block_size);
> - s->bitmap_size = ((s->block_size / (8 * 512)) + 511) & ~511;
> + s->block_size = be32_to_cpu(dyndisk_header->block_size);
> + s->bitmap_size = ((s->block_size / (8 * 512)) + 511) & ~511;
>
> - s->max_table_entries = be32_to_cpu(dyndisk_header->max_table_entries);
> - s->pagetable = g_malloc(s->max_table_entries * 4);
> + s->max_table_entries = be32_to_cpu(dyndisk_header->max_table_entries);
> + s->pagetable = g_malloc(s->max_table_entries * 4);
>
> - s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
> - if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
> - s->max_table_entries * 4) != s->max_table_entries * 4)
> - goto fail;
> + s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
> + if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
> + s->max_table_entries * 4) != s->max_table_entries * 4) {
> + goto fail;
> + }
>
> - s->free_data_block_offset =
> - (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511;
> + s->free_data_block_offset =
> + (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511;
>
> - for (i = 0; i < s->max_table_entries; i++) {
> - be32_to_cpus(&s->pagetable[i]);
> - if (s->pagetable[i] != 0xFFFFFFFF) {
> - int64_t next = (512 * (int64_t) s->pagetable[i]) +
> - s->bitmap_size + s->block_size;
> + for (i = 0; i < s->max_table_entries; i++) {
> + be32_to_cpus(&s->pagetable[i]);
> + if (s->pagetable[i] != 0xFFFFFFFF) {
> + int64_t next = (512 * (int64_t) s->pagetable[i]) +
> + s->bitmap_size + s->block_size;
>
> - if (next> s->free_data_block_offset)
> - s->free_data_block_offset = next;
> + if (next > s->free_data_block_offset) {
> + s->free_data_block_offset = next;
> + }
> + }
> }
> - }
>
> - s->last_bitmap_offset = (int64_t) -1;
> + s->last_bitmap_offset = (int64_t) -1;
>
> #ifdef CACHE
> - s->pageentry_u8 = g_malloc(512);
> - s->pageentry_u32 = s->pageentry_u8;
> - s->pageentry_u16 = s->pageentry_u8;
> - s->last_pagetable = -1;
> + s->pageentry_u8 = g_malloc(512);
> + s->pageentry_u32 = s->pageentry_u8;
> + s->pageentry_u16 = s->pageentry_u8;
> + s->last_pagetable = -1;
> #endif
> + }
>
> qemu_co_mutex_init(&s->lock);
>
> @@ -395,7 +414,11 @@ static int vpc_read(BlockDriverState *bs, int64_t sector_num,
> int ret;
> int64_t offset;
> int64_t sectors, sectors_per_block;
> + struct vhd_footer *footer = (struct vhd_footer *) s->footer_buf;
>
> + if (cpu_to_be32(footer->type) == VHD_FIXED) {
> + return bdrv_read(bs->file, sector_num, buf, nb_sectors);
> + }
> while (nb_sectors > 0) {
> offset = get_sector_offset(bs, sector_num, 0);
>
> @@ -440,7 +463,11 @@ static int vpc_write(BlockDriverState *bs, int64_t sector_num,
> int64_t offset;
> int64_t sectors, sectors_per_block;
> int ret;
> + struct vhd_footer *footer = (struct vhd_footer *) s->footer_buf;
>
> + if (cpu_to_be32(footer->type) == VHD_FIXED) {
> + return bdrv_write(bs->file, sector_num, buf, nb_sectors);
> + }
> while (nb_sectors > 0) {
> offset = get_sector_offset(bs, sector_num, 1);
>
> @@ -533,70 +560,14 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
> return 0;
> }
>
> -static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
> {
> - uint8_t buf[1024];
> - struct vhd_footer* footer = (struct vhd_footer*) buf;
> struct vhd_dyndisk_header* dyndisk_header =
> (struct vhd_dyndisk_header*) buf;
> - int fd, i;
> - uint16_t cyls = 0;
> - uint8_t heads = 0;
> - uint8_t secs_per_cyl = 0;
> size_t block_size, num_bat_entries;
> - int64_t total_sectors = 0;
> + int i;
> int ret = -EIO;
>
> - // Read out options
> - total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
> - BDRV_SECTOR_SIZE;
> -
> - // Create the file
> - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> - if (fd < 0)
> - return -EIO;
> -
> - /* Calculate matching total_size and geometry. Increase the number of
> - sectors requested until we get enough (or fail). */
> - 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 fail;
> - }
> - }
> - total_sectors = (int64_t) cyls * heads * secs_per_cyl;
> -
> - // Prepare the Hard Disk Footer
> - memset(buf, 0, 1024);
> -
> - memcpy(footer->creator, "conectix", 8);
> - // TODO Check if "qemu" creator_app is ok for VPC
> - memcpy(footer->creator_app, "qemu", 4);
> - memcpy(footer->creator_os, "Wi2k", 4);
> -
> - footer->features = be32_to_cpu(0x02);
> - footer->version = be32_to_cpu(0x00010000);
> - footer->data_offset = be64_to_cpu(HEADER_SIZE);
> - footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
> -
> - // Version of Virtual PC 2007
> - footer->major = be16_to_cpu(0x0005);
> - footer->minor =be16_to_cpu(0x0003);
> -
> - footer->orig_size = be64_to_cpu(total_sectors * 512);
> - footer->size = be64_to_cpu(total_sectors * 512);
> -
> - footer->cyls = be16_to_cpu(cyls);
> - footer->heads = heads;
> - footer->secs_per_cyl = secs_per_cyl;
> -
> - footer->type = be32_to_cpu(VHD_DYNAMIC);
> -
> - // TODO uuid is missing
> -
> - footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
> -
> // Write the footer (twice: at the beginning and at the end)
> block_size = 0x200000;
> num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
> @@ -624,7 +595,6 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
> }
> }
>
> -
> // Prepare the Dynamic Disk Header
> memset(buf, 0, 1024);
>
> @@ -653,6 +623,130 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
> ret = 0;
>
> fail:
> + return ret;
> +}
> +
> +static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
> +{
> + int ret = -EIO;
> +
> + /* Add footer to total size */
> + total_size += 512;
> + if (ftruncate(fd, total_size) != 0) {
> + ret = -errno;
> + goto fail;
> + }
> + if (lseek(fd, -512, SEEK_END) < 0) {
> + goto fail;
> + }
> + if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
> + goto fail;
> + }
> +
> + ret = 0;
> +
> + fail:
> + return ret;
> +}
> +
> +static int vpc_create(const char *filename, QEMUOptionParameter *options)
> +{
> + uint8_t buf[1024];
> + struct vhd_footer *footer = (struct vhd_footer *) buf;
> + QEMUOptionParameter *disk_type_param;
> + int fd, i;
> + uint16_t cyls = 0;
> + uint8_t heads = 0;
> + uint8_t secs_per_cyl = 0;
> + int64_t total_sectors;
> + int64_t total_size;
> + int disk_type;
> + int ret = -EIO;
> +
> + /* Read out options */
> + total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
> +
> + disk_type_param = get_option_parameter(options, BLOCK_OPT_SUBFMT);
> + if (disk_type_param && disk_type_param->value.s) {
> + if (!strcmp(disk_type_param->value.s, "dynamic")) {
> + disk_type = VHD_DYNAMIC;
> + } else if (!strcmp(disk_type_param->value.s, "fixed")) {
> + disk_type = VHD_FIXED;
> + } else {
> + return -EINVAL;
> + }
> + } else {
> + disk_type = VHD_DYNAMIC;
> + }
> +
> + /* Create the file */
> + fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> + if (fd < 0) {
> + return -EIO;
> + }
> +
> + total_sectors = total_size / BDRV_SECTOR_SIZE;
> + if (disk_type == VHD_DYNAMIC) {
> + /* Calculate matching total_size and geometry. Increase the number of
> + sectors requested until we get enough (or fail). */
> + for (i = 0; total_sectors > (int64_t)cyls * heads * secs_per_cyl;
> + i++) {
> + if (calculate_geometry(total_sectors + i,
> + &cyls, &heads, &secs_per_cyl)) {
> + goto fail;
Somehow you lost the ret = -EFBIG here.
Otherwise the patch looks good enough for me.
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-02-06 15:46 ` Kevin Wolf
@ 2012-02-06 16:22 ` Charles Arnold
2012-02-06 16:51 ` Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Charles Arnold @ 2012-02-06 16:22 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, afaerber
>>> On 2/6/2012 at 08:46 AM, in message <4F2FF5B9.9090404@redhat.com>, Kevin Wolf
<kwolf@redhat.com> wrote:
>
> Somehow you lost the ret = -EFBIG here.
>
> Otherwise the patch looks good enough for me.
>
> Kevin
Thanks Kevin. Here is the revised patch with just this fix.
- Charles
The Virtual Hard Disk Image Format Specification allows for three
types of hard disk formats, Fixed, Dynamic, and Differencing. Qemu
currently only supports Dynamic disks. This patch adds support for
the Fixed Disk format.
Usage:
Example 1: qemu-img create -f vpc -o type=fixed <filename> [size]
Example 2: qemu-img convert -O vpc -o type=fixed <input filename> <output filename>
While it is also allowed to specify '-o type=dynamic', the default disk type
remains Dynamic and is what is used when the type is left unspecified.
Signed-off-by: Charles Arnold <carnold@suse.com>
diff --git a/block/vpc.c b/block/vpc.c
index 89a5ee2..db6b14b 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -161,13 +161,27 @@ static int vpc_open(BlockDriverState *bs, int flags)
uint8_t buf[HEADER_SIZE];
uint32_t checksum;
int err = -1;
+ int disk_type = VHD_DYNAMIC;
if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE)
goto fail;
footer = (struct vhd_footer*) s->footer_buf;
- if (strncmp(footer->creator, "conectix", 8))
- goto fail;
+ if (strncmp(footer->creator, "conectix", 8)) {
+ int64_t offset = bdrv_getlength(bs->file);
+ if (offset < HEADER_SIZE) {
+ goto fail;
+ }
+ /* If a fixed disk, the footer is found only at the end of the file */
+ if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE)
+ != HEADER_SIZE) {
+ goto fail;
+ }
+ if (strncmp(footer->creator, "conectix", 8)) {
+ goto fail;
+ }
+ disk_type = VHD_FIXED;
+ }
checksum = be32_to_cpu(footer->checksum);
footer->checksum = 0;
@@ -186,49 +200,54 @@ static int vpc_open(BlockDriverState *bs, int flags)
goto fail;
}
- if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, HEADER_SIZE)
- != HEADER_SIZE)
- goto fail;
-
- dyndisk_header = (struct vhd_dyndisk_header*) buf;
+ if (disk_type == VHD_DYNAMIC) {
+ if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf,
+ HEADER_SIZE) != HEADER_SIZE) {
+ goto fail;
+ }
- if (strncmp(dyndisk_header->magic, "cxsparse", 8))
- goto fail;
+ dyndisk_header = (struct vhd_dyndisk_header *) buf;
+ if (strncmp(dyndisk_header->magic, "cxsparse", 8)) {
+ goto fail;
+ }
- s->block_size = be32_to_cpu(dyndisk_header->block_size);
- s->bitmap_size = ((s->block_size / (8 * 512)) + 511) & ~511;
+ s->block_size = be32_to_cpu(dyndisk_header->block_size);
+ s->bitmap_size = ((s->block_size / (8 * 512)) + 511) & ~511;
- s->max_table_entries = be32_to_cpu(dyndisk_header->max_table_entries);
- s->pagetable = g_malloc(s->max_table_entries * 4);
+ s->max_table_entries = be32_to_cpu(dyndisk_header->max_table_entries);
+ s->pagetable = g_malloc(s->max_table_entries * 4);
- s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
- if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
- s->max_table_entries * 4) != s->max_table_entries * 4)
- goto fail;
+ s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
+ if (bdrv_pread(bs->file, s->bat_offset, s->pagetable,
+ s->max_table_entries * 4) != s->max_table_entries * 4) {
+ goto fail;
+ }
- s->free_data_block_offset =
- (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511;
+ s->free_data_block_offset =
+ (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511;
- for (i = 0; i < s->max_table_entries; i++) {
- be32_to_cpus(&s->pagetable[i]);
- if (s->pagetable[i] != 0xFFFFFFFF) {
- int64_t next = (512 * (int64_t) s->pagetable[i]) +
- s->bitmap_size + s->block_size;
+ for (i = 0; i < s->max_table_entries; i++) {
+ be32_to_cpus(&s->pagetable[i]);
+ if (s->pagetable[i] != 0xFFFFFFFF) {
+ int64_t next = (512 * (int64_t) s->pagetable[i]) +
+ s->bitmap_size + s->block_size;
- if (next> s->free_data_block_offset)
- s->free_data_block_offset = next;
+ if (next > s->free_data_block_offset) {
+ s->free_data_block_offset = next;
+ }
+ }
}
- }
- s->last_bitmap_offset = (int64_t) -1;
+ s->last_bitmap_offset = (int64_t) -1;
#ifdef CACHE
- s->pageentry_u8 = g_malloc(512);
- s->pageentry_u32 = s->pageentry_u8;
- s->pageentry_u16 = s->pageentry_u8;
- s->last_pagetable = -1;
+ s->pageentry_u8 = g_malloc(512);
+ s->pageentry_u32 = s->pageentry_u8;
+ s->pageentry_u16 = s->pageentry_u8;
+ s->last_pagetable = -1;
#endif
+ }
qemu_co_mutex_init(&s->lock);
@@ -395,7 +414,11 @@ static int vpc_read(BlockDriverState *bs, int64_t sector_num,
int ret;
int64_t offset;
int64_t sectors, sectors_per_block;
+ struct vhd_footer *footer = (struct vhd_footer *) s->footer_buf;
+ if (cpu_to_be32(footer->type) == VHD_FIXED) {
+ return bdrv_read(bs->file, sector_num, buf, nb_sectors);
+ }
while (nb_sectors > 0) {
offset = get_sector_offset(bs, sector_num, 0);
@@ -440,7 +463,11 @@ static int vpc_write(BlockDriverState *bs, int64_t sector_num,
int64_t offset;
int64_t sectors, sectors_per_block;
int ret;
+ struct vhd_footer *footer = (struct vhd_footer *) s->footer_buf;
+ if (cpu_to_be32(footer->type) == VHD_FIXED) {
+ return bdrv_write(bs->file, sector_num, buf, nb_sectors);
+ }
while (nb_sectors > 0) {
offset = get_sector_offset(bs, sector_num, 1);
@@ -533,70 +560,14 @@ static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
return 0;
}
-static int vpc_create(const char *filename, QEMUOptionParameter *options)
+static int create_dynamic_disk(int fd, uint8_t *buf, int64_t total_sectors)
{
- uint8_t buf[1024];
- struct vhd_footer* footer = (struct vhd_footer*) buf;
struct vhd_dyndisk_header* dyndisk_header =
(struct vhd_dyndisk_header*) buf;
- int fd, i;
- uint16_t cyls = 0;
- uint8_t heads = 0;
- uint8_t secs_per_cyl = 0;
size_t block_size, num_bat_entries;
- int64_t total_sectors = 0;
+ int i;
int ret = -EIO;
- // Read out options
- total_sectors = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n /
- BDRV_SECTOR_SIZE;
-
- // Create the file
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
- if (fd < 0)
- return -EIO;
-
- /* Calculate matching total_size and geometry. Increase the number of
- sectors requested until we get enough (or fail). */
- 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 fail;
- }
- }
- total_sectors = (int64_t) cyls * heads * secs_per_cyl;
-
- // Prepare the Hard Disk Footer
- memset(buf, 0, 1024);
-
- memcpy(footer->creator, "conectix", 8);
- // TODO Check if "qemu" creator_app is ok for VPC
- memcpy(footer->creator_app, "qemu", 4);
- memcpy(footer->creator_os, "Wi2k", 4);
-
- footer->features = be32_to_cpu(0x02);
- footer->version = be32_to_cpu(0x00010000);
- footer->data_offset = be64_to_cpu(HEADER_SIZE);
- footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
-
- // Version of Virtual PC 2007
- footer->major = be16_to_cpu(0x0005);
- footer->minor =be16_to_cpu(0x0003);
-
- footer->orig_size = be64_to_cpu(total_sectors * 512);
- footer->size = be64_to_cpu(total_sectors * 512);
-
- footer->cyls = be16_to_cpu(cyls);
- footer->heads = heads;
- footer->secs_per_cyl = secs_per_cyl;
-
- footer->type = be32_to_cpu(VHD_DYNAMIC);
-
- // TODO uuid is missing
-
- footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
-
// Write the footer (twice: at the beginning and at the end)
block_size = 0x200000;
num_bat_entries = (total_sectors + block_size / 512) / (block_size / 512);
@@ -624,7 +595,6 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
}
}
-
// Prepare the Dynamic Disk Header
memset(buf, 0, 1024);
@@ -653,6 +623,132 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
ret = 0;
fail:
+ return ret;
+}
+
+static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
+{
+ int ret = -EIO;
+
+ /* Add footer to total size */
+ total_size += 512;
+ if (ftruncate(fd, total_size) != 0) {
+ ret = -errno;
+ goto fail;
+ }
+ if (lseek(fd, -512, SEEK_END) < 0) {
+ goto fail;
+ }
+ if (write(fd, buf, HEADER_SIZE) != HEADER_SIZE) {
+ goto fail;
+ }
+
+ ret = 0;
+
+ fail:
+ return ret;
+}
+
+static int vpc_create(const char *filename, QEMUOptionParameter *options)
+{
+ uint8_t buf[1024];
+ struct vhd_footer *footer = (struct vhd_footer *) buf;
+ QEMUOptionParameter *disk_type_param;
+ int fd, i;
+ uint16_t cyls = 0;
+ uint8_t heads = 0;
+ uint8_t secs_per_cyl = 0;
+ int64_t total_sectors;
+ int64_t total_size;
+ int disk_type;
+ int ret = -EIO;
+
+ /* Read out options */
+ total_size = get_option_parameter(options, BLOCK_OPT_SIZE)->value.n;
+
+ disk_type_param = get_option_parameter(options, BLOCK_OPT_SUBFMT);
+ if (disk_type_param && disk_type_param->value.s) {
+ if (!strcmp(disk_type_param->value.s, "dynamic")) {
+ disk_type = VHD_DYNAMIC;
+ } else if (!strcmp(disk_type_param->value.s, "fixed")) {
+ disk_type = VHD_FIXED;
+ } else {
+ return -EINVAL;
+ }
+ } else {
+ disk_type = VHD_DYNAMIC;
+ }
+
+ /* Create the file */
+ fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
+ if (fd < 0) {
+ return -EIO;
+ }
+
+ total_sectors = total_size / BDRV_SECTOR_SIZE;
+ if (disk_type == VHD_DYNAMIC) {
+ /* Calculate matching total_size and geometry. Increase the number of
+ sectors requested until we get enough (or fail). */
+ 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 fail;
+ }
+ }
+ } else {
+ if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
+ ret = -EFBIG;
+ goto fail;
+ }
+ }
+ total_sectors = (int64_t) cyls * heads * secs_per_cyl;
+
+ /* Prepare the Hard Disk Footer */
+ memset(buf, 0, 1024);
+
+ memcpy(footer->creator, "conectix", 8);
+ /* TODO Check if "qemu" creator_app is ok for VPC */
+ memcpy(footer->creator_app, "qemu", 4);
+ memcpy(footer->creator_os, "Wi2k", 4);
+
+ footer->features = be32_to_cpu(0x02);
+ footer->version = be32_to_cpu(0x00010000);
+ if (disk_type == VHD_DYNAMIC) {
+ footer->data_offset = be64_to_cpu(HEADER_SIZE);
+ } else {
+ footer->data_offset = be64_to_cpu(0xFFFFFFFFFFFFFFFFULL);
+ }
+ footer->timestamp = be32_to_cpu(time(NULL) - VHD_TIMESTAMP_BASE);
+
+ /* Version of Virtual PC 2007 */
+ footer->major = be16_to_cpu(0x0005);
+ footer->minor = be16_to_cpu(0x0003);
+ if (disk_type == VHD_DYNAMIC) {
+ footer->orig_size = be64_to_cpu(total_sectors * 512);
+ footer->size = be64_to_cpu(total_sectors * 512);
+ } else {
+ footer->orig_size = be64_to_cpu(total_size);
+ footer->size = be64_to_cpu(total_size);
+ }
+ footer->cyls = be16_to_cpu(cyls);
+ footer->heads = heads;
+ footer->secs_per_cyl = secs_per_cyl;
+
+ footer->type = be32_to_cpu(disk_type);
+
+ /* TODO uuid is missing */
+
+ footer->checksum = be32_to_cpu(vpc_checksum(buf, HEADER_SIZE));
+
+ if (disk_type == VHD_DYNAMIC) {
+ ret = create_dynamic_disk(fd, buf, total_sectors);
+ } else {
+ ret = create_fixed_disk(fd, buf, total_size);
+ }
+
+ fail:
close(fd);
return ret;
}
@@ -675,6 +771,13 @@ static QEMUOptionParameter vpc_create_options[] = {
.type = OPT_SIZE,
.help = "Virtual disk size"
},
+ {
+ .name = BLOCK_OPT_SUBFMT,
+ .type = OPT_STRING,
+ .help =
+ "Type of virtual hard disk format. Supported formats are "
+ "{dynamic (default) | fixed} "
+ },
{ NULL }
};
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-02-06 16:22 ` Charles Arnold
@ 2012-02-06 16:51 ` Kevin Wolf
2012-02-06 23:48 ` Charles Arnold
0 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2012-02-06 16:51 UTC (permalink / raw)
To: Charles Arnold; +Cc: qemu-devel, afaerber
Am 06.02.2012 17:22, schrieb Charles Arnold:
>>>> On 2/6/2012 at 08:46 AM, in message <4F2FF5B9.9090404@redhat.com>, Kevin Wolf
> <kwolf@redhat.com> wrote:
>>
>> Somehow you lost the ret = -EFBIG here.
>>
>> Otherwise the patch looks good enough for me.
>>
>> Kevin
>
> Thanks Kevin. Here is the revised patch with just this fix.
> - Charles
Thanks, applied to the block branch.
I have one question left, though:
> + total_sectors = total_size / BDRV_SECTOR_SIZE;
> + if (disk_type == VHD_DYNAMIC) {
> + /* Calculate matching total_size and geometry. Increase the number of
> + sectors requested until we get enough (or fail). */
> + 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 fail;
> + }
> + }
> + } else {
> + if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
> + ret = -EFBIG;
> + goto fail;
> + }
> + }
What's the reason that we need to do things differently here depending
on the subformat? Dynamic disks round up the size so that during image
conversion images won't be truncated. For fixed images,
calculate_geometry can round down, so don't fixed image have the same
problem?
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
2012-02-06 16:51 ` Kevin Wolf
@ 2012-02-06 23:48 ` Charles Arnold
2012-02-07 9:22 ` [Qemu-devel] [PATCH] vpc: Round up image size during fixed image creation Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Charles Arnold @ 2012-02-06 23:48 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, afaerber
>>> On 2/6/2012 at 09:51 AM, in message <4F3004FC.7070802@redhat.com>, Kevin Wolf
<kwolf@redhat.com> wrote:
> Am 06.02.2012 17:22, schrieb Charles Arnold:
>>>>> On 2/6/2012 at 08:46 AM, in message <4F2FF5B9.9090404@redhat.com>, Kevin Wolf
>> <kwolf@redhat.com> wrote:
>>>
>>> Somehow you lost the ret = -EFBIG here.
>>>
>>> Otherwise the patch looks good enough for me.
>>>
>>> Kevin
>>
>> Thanks Kevin. Here is the revised patch with just this fix.
>> - Charles
>
> Thanks, applied to the block branch.
>
> I have one question left, though:
>
>> + total_sectors = total_size / BDRV_SECTOR_SIZE;
>> + if (disk_type == VHD_DYNAMIC) {
>> + /* Calculate matching total_size and geometry. Increase the number
> of
>> + sectors requested until we get enough (or fail). */
>> + 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 fail;
>> + }
>> + }
>> + } else {
>> + if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
>> + ret = -EFBIG;
>> + goto fail;
>> + }
>> + }
>
> What's the reason that we need to do things differently here depending
> on the subformat? Dynamic disks round up the size so that during image
> conversion images won't be truncated. For fixed images,
> calculate_geometry can round down, so don't fixed image have the same
> problem?
Yes. In my testing I was simply creating a fixed disk that would appear exactly as
it does when an equivalent size was created on windows. I did not factor in
the rounding needed on convert in order to get the right number of sectors
to cover the total (and somewhat arbitrary) size of the disk.
Combining them means that we will usually round up a little bit even on create
which I suppose is fine.
- Charles
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH] vpc: Round up image size during fixed image creation
2012-02-06 23:48 ` Charles Arnold
@ 2012-02-07 9:22 ` Kevin Wolf
0 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2012-02-07 9:22 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, carnold
The geometry calculation algorithm from the VHD spec rounds the image
size down if it doesn't exactly match a geometry. During image
conversion, this causes the image to be truncated. For dynamic images,
we already have code in place to round up instead, let's do the same for
fixed images.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/vpc.c | 23 ++++++++++-------------
1 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/block/vpc.c b/block/vpc.c
index db6b14b..6b4816f 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -685,24 +685,21 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
return -EIO;
}
+ /*
+ * 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.
+ */
total_sectors = total_size / BDRV_SECTOR_SIZE;
- if (disk_type == VHD_DYNAMIC) {
- /* Calculate matching total_size and geometry. Increase the number of
- sectors requested until we get enough (or fail). */
- 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 fail;
- }
- }
- } else {
- if (calculate_geometry(total_sectors, &cyls, &heads, &secs_per_cyl)) {
+ 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 fail;
}
}
+
total_sectors = (int64_t) cyls * heads * secs_per_cyl;
/* Prepare the Hard Disk Footer */
--
1.7.6.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-02-07 9:19 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-31 19:03 [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type Charles Arnold
2012-01-31 22:08 ` Andreas Färber
2012-01-31 23:04 ` Charles Arnold
2012-01-31 23:15 ` Andreas Färber
2012-02-01 12:15 ` Kevin Wolf
2012-02-01 16:51 ` Charles Arnold
2012-02-01 17:09 ` Stefan Weil
2012-02-02 8:58 ` Kevin Wolf
2012-02-03 0:16 ` Charles Arnold
2012-02-06 15:46 ` Kevin Wolf
2012-02-06 16:22 ` Charles Arnold
2012-02-06 16:51 ` Kevin Wolf
2012-02-06 23:48 ` Charles Arnold
2012-02-07 9:22 ` [Qemu-devel] [PATCH] vpc: Round up image size during fixed image creation Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.