[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type
|
From: |
Kevin Wolf |
|
Subject: |
Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type |
|
Date: |
Thu, 02 Feb 2012 09:58:09 +0100 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20111222 Thunderbird/9.0 |
Am 01.02.2012 17:51, schrieb Charles Arnold:
>>>> On 2/1/2012 at 05:15 AM, in message <address@hidden>, Kevin Wolf
> <address@hidden> 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 <address@hidden>
>>> @@ -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
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type, Kevin Wolf, 2012/02/01
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type, Charles Arnold, 2012/02/01
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type, Stefan Weil, 2012/02/01
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type,
Kevin Wolf <=
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type, Charles Arnold, 2012/02/02
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type, Kevin Wolf, 2012/02/06
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type, Charles Arnold, 2012/02/06
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type, Kevin Wolf, 2012/02/06
- Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type, Charles Arnold, 2012/02/06
- [Qemu-devel] [PATCH] vpc: Round up image size during fixed image creation, Kevin Wolf, 2012/02/07