From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:45179) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsZ2Z-00018a-6H for qemu-devel@nongnu.org; Wed, 01 Feb 2012 07:11:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RsZ2U-0002a4-7B for qemu-devel@nongnu.org; Wed, 01 Feb 2012 07:11:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52103) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RsZ2T-0002Zj-TG for qemu-devel@nongnu.org; Wed, 01 Feb 2012 07:11:50 -0500 Message-ID: <4F292CD0.20707@redhat.com> Date: Wed, 01 Feb 2012 13:15:12 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <4F27D8A00200009100079C3A@novprvoes0310.provo.novell.com> <4F286659.6060300@suse.de> <4F2811220200009100079C4A@novprvoes0310.provo.novell.com> In-Reply-To: <4F2811220200009100079C4A@novprvoes0310.provo.novell.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Charles Arnold Cc: =?ISO-8859-1?Q?Andreas_F=E4rber?= , qemu-devel@nongnu.org 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 [size] > Example 2: qemu-img convert -O vpc -o type=fixed > > 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 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