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

[Kevin Wolf]

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 <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>

I'll drop the casts while applying.

Kevin