Re: [Qemu-devel] [PATCH] vpc: Remove useless parameters in get_sector_offset()

[Peter Maydell]

On 13 July 2017 at 13:37, Kevin Wolf <address@hidden> wrote:
> All callers of get_sector_offset() pass write=false and err=NULL. Remove
> the parameters.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> Based on my block branch, specifically Peter's 'block/vpc.c: Handle write
> failures in get_image_offset()'
>
>  block/vpc.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/block/vpc.c b/block/vpc.c
> index bccc981..471f9ab 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -508,10 +508,9 @@ static inline int64_t get_image_offset(BlockDriverState 
> *bs, uint64_t offset,
>  }
>
>  static inline int64_t get_sector_offset(BlockDriverState *bs,
> -                                        int64_t sector_num, bool write,
> -                                        int *err)
> +                                        int64_t sector_num)
>  {
> -    return get_image_offset(bs, sector_num * BDRV_SECTOR_SIZE, write, err);
> +    return get_image_offset(bs, sector_num * BDRV_SECTOR_SIZE, false, NULL);
>  }
>
>  /*
> @@ -721,7 +720,7 @@ static int64_t coroutine_fn 
> vpc_co_get_block_status(BlockDriverState *bs,
>                 (sector_num << BDRV_SECTOR_BITS);
>      }
>
> -    offset = get_sector_offset(bs, sector_num, false, NULL);
> +    offset = get_sector_offset(bs, sector_num);
>      start = offset;
>      allocated = (offset != -1);
>      *pnum = 0;
> @@ -744,7 +743,7 @@ static int64_t coroutine_fn 
> vpc_co_get_block_status(BlockDriverState *bs,
>          if (nb_sectors == 0) {
>              break;
>          }
> -        offset = get_sector_offset(bs, sector_num, false, NULL);
> +        offset = get_sector_offset(bs, sector_num);
>      } while (offset == -1);
>
>      return 0;

Just to copy my remark over from the other thread:
this turns a function that could be used for writes into a function
that doesn't look like it cares whether it's used for reads or
writes but which will actually silently cause disk image
corruption if you do ever use it in a write codepath. That's
a bit of a bear trap as API design goes, which is why I opted
to add the 'err' argument to get_image_offset() rather than
just removing the 'write' argument.

thanks
-- PMM