Re: [Qemu-block] [PATCH v3 2/3] block: Fix target variable of BLKSECTGET ioctl

[Max Reitz]

On 20.01.2017 17:25, Eric Farman wrote:
> Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
> introduced a routine to call the kernel BLKSECTGET ioctl, which stores the
> result back to user space.  However, the size of the data returned depends
> on the routine handling the ioctl.  The (compat_)blkdev_ioctl returns a
> short, while sg_ioctl returns an int.  Thus, on big-endian systems, we can
> find ourselves accidentally shifting the result to a much larger value.
> (On s390x, a short is 16 bits while an int is 32 bits.)
> 
> Also, the two ioctl handlers return values in different scales (block
> returns sectors, while sg returns bytes), so some tweaking of the outputs
> is required such that hdev_get_max_transfer_length returns a value in a
> consistent set of units.
> 
> Signed-off-by: Eric Farman <address@hidden>
> ---
>  block/file-posix.c    | 17 ++++++++++-------
>  include/block/block.h |  1 +
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 28b47d9..9f83725 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -651,12 +651,15 @@ static void raw_reopen_abort(BDRVReopenState *state)
>      state->opaque = NULL;
>  }
>  
> -static int hdev_get_max_transfer_length(int fd)
> +static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
>  {
>  #ifdef BLKSECTGET
> -    int max_sectors = 0;
> -    if (ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
> -        return max_sectors;
> +    int max_bytes = 0;
> +    short max_sectors = 0;
> +    if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) {
> +        return max_bytes;
> +    } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
> +        return max_sectors << BDRV_SECTOR_BITS;
>      } else {
>          return -errno;
>      }
> @@ -672,9 +675,9 @@ static void raw_refresh_limits(BlockDriverState *bs, 
> Error **errp)
>  
>      if (!fstat(s->fd, &st)) {
>          if (S_ISBLK(st.st_mode)) {
> -            int ret = hdev_get_max_transfer_length(s->fd);
> -            if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
> -                bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
> +            int ret = hdev_get_max_transfer_length(bs, s->fd);
> +            if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> +                bs->bl.max_transfer = pow2floor(ret);
>              }
>          }
>      }
> diff --git a/include/block/block.h b/include/block/block.h
> index 8b0dcda..4e81f20 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -116,6 +116,7 @@ typedef struct HDGeometry {
>  
>  #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>                                       INT_MAX >> BDRV_SECTOR_BITS)
> +#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)

I'd rather like it the other way round (i.e.

#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX)
#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >>
BDRV_SECTOR_BITS)

), but I don't suppose I can interest you in a respin, can I? Would it
be OK with you if I changed you commit when I apply it?

Max

>  
>  /*
>   * Allocation status flags
>

signature.asc
Description: OpenPGP digital signature