Re: [Qemu-block] [PATCH 3/5] vvfat: Replace bdrv_{read, write}() with bdrv_{pread, pwrite}()

[Kevin Wolf]

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 vvfat code, and they can be trivially replaced with the byte-based
> bdrv_pread() and bdrv_pwrite().
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block/vvfat.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 5f66787890..35c7e2761f 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1494,8 +1494,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t 
> sector_num,
>                  DLOG(fprintf(stderr, "sectors %" PRId64 "+%" PRId64
>                               " allocated\n", sector_num,
>                               n >> BDRV_SECTOR_BITS));
> -                if (bdrv_read(s->qcow, sector_num, buf + i * 0x200,
> -                              n >> BDRV_SECTOR_BITS)) {
> +                if (bdrv_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE,
> +                               buf + i * 0x200, n)) {

bdrv_pread() returns a positive number of bytes in the success case, so
this error check is wrong. (No real reason why it couldn't return 0, but
we would have to check and possibly update all callers, so we never did
that.)

>                      return -1;
>                  }
>                  i += (n >> BDRV_SECTOR_BITS) - 1;
> @@ -1983,7 +1983,8 @@ static uint32_t 
> get_cluster_count_for_direntry(BDRVVVFATState* s,
>                          if (res) {
>                              return -1;
>                          }
> -                        res = bdrv_write(s->qcow, offset, s->cluster_buffer, 
> 1);
> +                        res = bdrv_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
> +                                          s->cluster_buffer, 
> BDRV_SECTOR_SIZE);
>                          if (res) {

Same here, this needs to be res < 0 now.

>                              return -2;
>                          }
> @@ -3050,7 +3051,8 @@ DLOG(checkpoint());
>       * Use qcow backend. Commit later.
>       */
>  DLOG(fprintf(stderr, "Write to qcow backend: %d + %d\n", (int)sector_num, 
> nb_sectors));
> -    ret = bdrv_write(s->qcow, sector_num, buf, nb_sectors);
> +    ret = bdrv_pwrite(s->qcow, sector_num * BDRV_SECTOR_SIZE, buf,
> +                      nb_sectors * BDRV_SECTOR_SIZE);
>      if (ret < 0) {

This one is already correct.

>          fprintf(stderr, "Error writing to qcow backend\n");
>          return ret;

Kevin