Re: [Qemu-devel] [PATCH v3 2/5] raw-posix: Factor block size detection out of raw_probe_alignment()

[Thomas Huth]

On Fri,  5 Dec 2014 18:56:18 +0100
Ekaterina Tumanova <address@hidden> wrote:

> Put it in new probe_logical_blocksize().
> 
> Signed-off-by: Ekaterina Tumanova <address@hidden>
> ---
>  block/raw-posix.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index b1af77e..633d5bc 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -217,11 +217,35 @@ static int raw_normalize_devicepath(const char 
> **filename)
>  }
>  #endif
> 
> +/*
> + * Set logical block size via ioctl. On success return 0. Otherwise -errno.

s/Set/Get/

> + */
> +static int probe_logical_blocksize(int fd, unsigned int *sector_size)
> +{
> +    /* Try a few ioctls to get the right size */
> +#ifdef BLKSSZGET
> +    if (ioctl(fd, BLKSSZGET, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +#ifdef DKIOCGETBLOCKSIZE
> +    if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +#ifdef DIOCGSECTORSIZE
> +    if (ioctl(fd, DIOCGSECTORSIZE, sector_size) < 0) {
> +        return -errno;
> +    }
> +#endif
> +
> +    return 0;

Is "return 0" the right thing here? Or should this function rather
return an error code if no ioctl was available?
(I know you're not using the return value yet, but it might get
important later)

> +}
> +
>  static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
>      char *buf;
> -    unsigned int sector_size;
> 
>      /* For /dev/sg devices the alignment is not really used.
>         With buffered I/O, we don't have any restrictions. */
> @@ -231,25 +255,11 @@ static void raw_probe_alignment(BlockDriverState *bs, 
> int fd, Error **errp)
>          return;
>      }
> 
> -    /* Try a few ioctls to get the right size */
>      bs->request_alignment = 0;
>      s->buf_align = 0;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    probe_logical_blocksize(fd, &bs->request_alignment);

Can we be sure that the &bs->request_alignment is not changed in case
the ioctl failed? If not, it might be necessary to do something like
this instead:

    if (probe_logical_blocksize(fd, &bs->request_alignment) != 0) {
        bs->request_alignment = 0;
    }

 Thomas