[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/5] vdi: Replace bdrv_{read, write}() with bdrv
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}() |
Date: |
Tue, 30 Apr 2019 12:03:22 +0200 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
Am 29.04.2019 um 20:42 hat Alberto Garcia geschrieben:
> There's only a couple of bdrv_read() and bdrv_write() calls left in
> the vdi code, and they can be trivially replaced with the byte-based
> bdrv_pread() and bdrv_pwrite().
>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
> block/vdi.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index e1c42ad732..8d849b2754 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -384,7 +384,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
> int flags,
>
> logout("\n");
>
> - ret = bdrv_read(bs->file, 0, (uint8_t *)&header, 1);
> + ret = bdrv_pread(bs->file, 0, (uint8_t *)&header, sizeof(header));
Maybe worth adding this after the VdiHeader declaration?
QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);
> if (ret < 0) {
> goto fail;
> }
> @@ -484,8 +484,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options,
> int flags,
> goto fail;
> }
>
> - ret = bdrv_read(bs->file, s->bmap_sector, (uint8_t *)s->bmap,
> - bmap_size);
> + ret = bdrv_pread(bs->file, header.offset_bmap, (uint8_t *)s->bmap,
> + bmap_size * SECTOR_SIZE);
> if (ret < 0) {
> goto fail_free_bmap;
> }
> @@ -704,7 +704,7 @@ nonallocating_write:
> assert(VDI_IS_ALLOCATED(bmap_first));
> *header = s->header;
> vdi_header_to_le(header);
> - ret = bdrv_write(bs->file, 0, block, 1);
> + ret = bdrv_pwrite(bs->file, 0, block, sizeof(*block));
block is uint8_t*, so sizeof(*block) == 1. This is not what you want.
(qemu-iotests catches this pretty early.)
> g_free(block);
> block = NULL;
>
> @@ -722,7 +722,8 @@ nonallocating_write:
> base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
> logout("will write %u block map sectors starting from entry %u\n",
> n_sectors, bmap_first);
> - ret = bdrv_write(bs->file, offset, base, n_sectors);
> + ret = bdrv_pwrite(bs->file, offset * SECTOR_SIZE, base,
> + n_sectors * SECTOR_SIZE);
> }
>
> return ret;
Let's avoid returning a non-zero positive number in the success case
here. bdrv_driver_pwritev() checks ret == 0 for BDRV_REQ_FUA emulation,
and its callers don't expect positive results either.
Maybe we should actually assert() in bdrv_driver_preadv/pwritev() that
the result is <= 0.
Kevin
- [Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write(), Alberto Garcia, 2019/04/29
- [Qemu-devel] [PATCH 1/5] qcow2: Replace bdrv_write() with bdrv_pwrite(), Alberto Garcia, 2019/04/29
- [Qemu-devel] [PATCH 4/5] block: Remove bdrv_read() and bdrv_write(), Alberto Garcia, 2019/04/29
- [Qemu-devel] [PATCH 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(), Alberto Garcia, 2019/04/29
- Re: [Qemu-devel] [PATCH 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(),
Kevin Wolf <=
- [Qemu-devel] [PATCH 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}(), Alberto Garcia, 2019/04/29
- [Qemu-devel] [PATCH 5/5] qcow2: Remove BDRVQcow2State.cluster_sectors, Alberto Garcia, 2019/04/29
- Re: [Qemu-devel] [PATCH 0/5] Remove bdrv_read() and bdrv_write(), Eric Blake, 2019/04/29