From: "Andreas Färber" <afaerber@suse.de>
To: Charles Arnold <carnold@suse.com>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
Date: Tue, 31 Jan 2012 23:08:25 +0100 [thread overview]
Message-ID: <4F286659.6060300@suse.de> (raw)
In-Reply-To: <4F27D8A00200009100079C3A@novprvoes0310.provo.novell.com>
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
next prev parent reply other threads:[~2012-01-31 22:10 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F286659.6060300@suse.de \
--to=afaerber@suse.de \
--cc=carnold@suse.com \
--cc=kwolf@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.