Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
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 carn...@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] != 0x) { -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] != 0x) { +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; +
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
On 2/6/2012 at 08:46 AM, in message 4f2ff5b9.9090...@redhat.com, Kevin Wolf kw...@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 carn...@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] != 0x) { -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] != 0x) { +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; +
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
Am 06.02.2012 17:22, schrieb Charles Arnold: On 2/6/2012 at 08:46 AM, in message 4f2ff5b9.9090...@redhat.com, Kevin Wolf kw...@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
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
On 2/6/2012 at 09:51 AM, in message 4f3004fc.7070...@redhat.com, Kevin Wolf kw...@redhat.com wrote: Am 06.02.2012 17:22, schrieb Charles Arnold: On 2/6/2012 at 08:46 AM, in message 4f2ff5b9.9090...@redhat.com, Kevin Wolf kw...@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
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
Am 01.02.2012 17:51, schrieb Charles Arnold: On 2/1/2012 at 05:15 AM, in message 4f292cd0.20...@redhat.com, Kevin Wolf kw...@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 carn...@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(0xULL); 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 0x. (Windows initializes all 8 bytes of the field) Which is relevant when creating images (there we do set data_offset to 0x), 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
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
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 carn...@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] != 0x) { -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] != 0x) { +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;
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
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 carn...@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(0xULL); 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. +} +
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
On 2/1/2012 at 05:15 AM, in message 4f292cd0.20...@redhat.com, Kevin Wolf kw...@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 carn...@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(0xULL); 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
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
Am 01.02.2012 17:51, schrieb Charles Arnold: On 2/1/2012 at 05:15 AM, in message4f292cd0.20...@redhat.com, Kevin Wolf kw...@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=fixedfilename [size] Example 2: qemu-img convert -O vpc -o type=fixedinput 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 Arnoldcarn...@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(0xULL); 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 0x. (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*
[Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
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 carn...@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(0x); +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;
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
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 carn...@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(0x); 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,
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
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 carn...@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(0xULL); +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
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
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 carn...@suse.com Reviewed-by: Andreas Färber afaer...@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(0xULL); +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,