From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RTE2Y-0000ff-I7 for qemu-devel@nongnu.org; Wed, 23 Nov 2011 09:43:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RTE2X-0006fQ-7Z for qemu-devel@nongnu.org; Wed, 23 Nov 2011 09:43:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:64317) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RTE2W-0006fC-VV for qemu-devel@nongnu.org; Wed, 23 Nov 2011 09:43:09 -0500 Message-ID: <4ECD0735.9040306@redhat.com> Date: Wed, 23 Nov 2011 15:46:13 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <4ECD011B.8060805@free.fr> In-Reply-To: <4ECD011B.8060805@free.fr> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] Add basic read, write and create support for AMD SimNow HDD images. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Fran=E7ois_Revol?= Cc: Stefan Hajnoczi , qemu-devel@nongnu.org Am 23.11.2011 15:20, schrieb Fran=E7ois Revol: > Hi, >=20 > Le 01/12/2010 11:38, Stefan Hajnoczi a =E9crit : >> 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). >=20 > 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 02= 7 028 > Passed all 11 tests >=20 > But before I submit the patch again I'll rather have it written correct= ly. >=20 >>> +typedef struct BDRVHddState { >>> + uint8_t identify_data[SECTOR_SIZE]; >> >> Perhaps identify_data[] should be uint16_t since it gets casted on eve= ry use. >=20 > 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. >=20 >>> +static void padstr(char *str, const char *src, int len) >>> +{ >>> + int i, v; >>> + for(i =3D 0; i < len; i++) { >>> + if (*src) >>> + v =3D *src++; >>> + else >>> + v =3D ' '; >>> + str[i^1] =3D v; >>> + } >>> +} >> >> This function is confusing, it uses int v instead of char. The name >> padstr() doesn't hint that it also byteswaps the string. >=20 > 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). >=20 > 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 *f= ilename) >> >> 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. >=20 > 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. >=20 >>> + if (bdrv_read(bs->file, 0, s->identify_data, 1) < 0) { >>> + goto fail; >>> + } >> >> We're assuming that BDRV_SECTOR_SIZE =3D=3D SECTOR_SIZE =3D=3D 512 thr= oughout >> 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. >=20 > Right, though the code would not work for SECTOR_SIZE !=3D 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 =3D -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. >=20 > 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