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
