Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/5] vdi: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> ---
>   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 <address@hidden>

>           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