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 <berto@igalia.com>
---
block/vdi.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index e1c42ad732..9caeb50dd1 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -171,6 +171,8 @@ typedef struct {
uint64_t unused2[7];
} QEMU_PACKED VdiHeader;
+QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);
+
typedef struct {
/* The block map entries are little endian (even in memory). */
uint32_t *bmap;
@@ -384,7 +386,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));
if (ret < 0) {
goto fail;
}
@@ -484,8 +486,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 +706,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(VdiHeader));
g_free(block);
block = NULL;
@@ -722,10 +724,11 @@ 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;
+ return ret < 0 ? ret : 0;
}
static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
--
2.11.0
01.05.2019 21:13, Alberto Garcia wrote:
> 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 <berto@igalia.com>
> ---
> block/vdi.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index e1c42ad732..9caeb50dd1 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -171,6 +171,8 @@ typedef struct {
> uint64_t unused2[7];
> } QEMU_PACKED VdiHeader;
>
> +QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);
> +
> typedef struct {
> /* The block map entries are little endian (even in memory). */
> uint32_t *bmap;
> @@ -384,7 +386,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));
bdrv_pread parameter buf parameter is void, so (uint8_t *) conversion is not needed
(and even confusing)
> if (ret < 0) {
> goto fail;
> }
> @@ -484,8 +486,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);
and here
> if (ret < 0) {
> goto fail_free_bmap;
> }
> @@ -704,7 +706,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(VdiHeader));
maybe, more self-descriptive:
ret = bdrv_pwrite(bs->file, 0, header, sizeof(*header));
with at least extra conversion dropped:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> g_free(block);
> block = NULL;
>
> @@ -722,10 +724,11 @@ 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;
> + return ret < 0 ? ret : 0;
> }
>
> static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
>
--
Best regards,
Vladimir
Am 06.05.2019 um 18:38 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 01.05.2019 21:13, Alberto Garcia wrote:
> > 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 <berto@igalia.com>
> > ---
> > block/vdi.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/block/vdi.c b/block/vdi.c
> > index e1c42ad732..9caeb50dd1 100644
> > --- a/block/vdi.c
> > +++ b/block/vdi.c
> > @@ -171,6 +171,8 @@ typedef struct {
> > uint64_t unused2[7];
> > } QEMU_PACKED VdiHeader;
> >
> > +QEMU_BUILD_BUG_ON(sizeof(VdiHeader) != 512);
> > +
> > typedef struct {
> > /* The block map entries are little endian (even in memory). */
> > uint32_t *bmap;
> > @@ -384,7 +386,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));
>
> bdrv_pread parameter buf parameter is void, so (uint8_t *) conversion is not needed
> (and even confusing)
>
> > if (ret < 0) {
> > goto fail;
> > }
> > @@ -484,8 +486,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);
>
> and here
>
> > if (ret < 0) {
> > goto fail_free_bmap;
> > }
> > @@ -704,7 +706,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(VdiHeader));
>
> maybe, more self-descriptive:
> ret = bdrv_pwrite(bs->file, 0, header, sizeof(*header));
>
>
> with at least extra conversion dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
I'll drop the casts while applying.
Kevin
© 2016 - 2025 Red Hat, Inc.