Re: [Qemu-devel] [PATCH] block: Add support for vpc Fixed Disk type

[Stefan Weil <sw@weilnetz.de>]

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