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 <far...@linux.vnet.ibm.com>
> ---
> 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
