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

[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 ?

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.

>> +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.

> 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

>> +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.

>> +    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.

>> +    /* 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.

François.