Re: [Qemu-devel] [PATCH] Add basic read, write and create support for AMD SimNow HDD images.

[Kevin Wolf]

Am 23.11.2011 15:20, schrieb François Revol:
> Hi,
> 
> Le 01/12/2010 11:38, Stefan Hajnoczi a écrit :
>> This block driver does not implement the asynchronous APIs
>> (bdrv_aio_writev, bdrv_aio_readv, bdrv_aio_flush) which are necessary
>> for running a VM properly.  Some block drivers are currently written
>> without async support and that limits them to being used as qemu-img
>> formats.  It's a bad idea to run a VM with these block drivers because
>> I/O will block the VM from making progress (it is synchronous).
> 
> I'm digging old stuff at the moment...
> It seems to be the coroutine calls recently introduced makes aio
> optional, does it ?

Yes. In fact, if you have co_* functions, aio_* will never be called
because the coroutine ones are always preferred.

> I added co versions of the calls in the code and it seems to work.
> It passes the qemu-iotests:
> Not run: 006 007 013 014 015 016 017 018 019 020 022 023 024 025 026 027 028
> Passed all 11 tests
> 
> But before I submit the patch again I'll rather have it written correctly.
> 
>>> +typedef struct BDRVHddState {
>>> +    uint8_t identify_data[SECTOR_SIZE];
>>
>> Perhaps identify_data[] should be uint16_t since it gets casted on every use.
> 
> I tried doing so but you end up adding many other casts everywhere
> and another #define to ...SIZE/sizeof(uint16_t) which doesn't look
> better though.

Yeah, I can see that there are many byte accesses as well. Probably best
to leave it as it is.

> 
>>> +static void padstr(char *str, const char *src, int len)
>>> +{
>>> +    int i, v;
>>> +    for(i = 0; i < len; i++) {
>>> +        if (*src)
>>> +            v = *src++;
>>> +        else
>>> +            v = ' ';
>>> +        str[i^1] = v;
>>> +    }
>>> +}
>>
>> This function is confusing, it uses int v instead of char.  The name
>> padstr() doesn't hint that it also byteswaps the string.
> 
> Well it was copied verbatim from another file.
> I now added a comment mentioning it.

Should be a common helper function somewhere then. Duplicating code is
always bad.

>> QEMU coding style uses {} even for one-line if statement bodies
>> (Section 4 in the CODING_STYLE file).
> 
> Well it was copied verbatim from another file. :P

While moving it to a common place, fix the code style. ;-)

>>> +static int hdd_probe(const uint8_t *buf, int buf_size, const char 
>>> *filename)
>>
>> HDD has no magic by which to identify valid files.  We need to avoid
>> false positives because existing image files could be corrupted or VMs
>> at least made unbootable.  Although using filename extensions to test
>> for formats is lame, in this case I don't think we have another
>> choice.
> 
> I suppose so, I didn't look any further but apart from checking the
> geometry validity at several places in the header there is not much to
> check for.

We should make sure to return a very low value so that any other
matching format will take precedence.

Or we could consider hdd_probe() { return 0; } and require the user to
specify the format explicitly. Would be a bit nasty to use, though.

> 
>>> +    if (bdrv_read(bs->file, 0, s->identify_data, 1) < 0) {
>>> +        goto fail;
>>> +    }
>>
>> We're assuming that BDRV_SECTOR_SIZE == SECTOR_SIZE == 512 throughout
>> the code.  It would be safer to explicitly calculate against
>> BDRV_SECTOR_SIZE.  It would be clearer to rename SECTOR_SIZE to
>> ATA_IDENTIFY_SIZE.
> 
> Right, though the code would not work for SECTOR_SIZE != 512 since it's
> the size of the header, so having it defined to something else would
> make blocks unaligned anyway.
> I'll use other defines but also an #error if SECTOR_SIZE doesn't fit to
> make sure people don't try things without noticing.

I think QEMU_BUILD_BUG_ON() is what you're looking for.

>>> +    /* TODO: specs says it can grow, so no need to always do this */
>>> +    if (static_image) {
>>> +        if (ftruncate(fd, sizeof(header) + blocks * SECTOR_SIZE)) {
>>> +            result = -errno;
>>> +        }
>>> +    }
>>
>> Is there an issue with leaving ftruncate() out?  I don't see a reason
>> to truncate.  If we want to preallocate then ftruncate() isn't
>> appropriate anyway.
> 
> Right, it doesn't write zeroes on ext2 anyway.
> I'd have to test without it first.

Please use bdrv_* functions instead of POSIX ones to create the image.

Kevin