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

[Markus Armbruster]

Ekaterina Tumanova <tuman...@linux.vnet.ibm.com> writes:

> Put it in new probe_logical_blocksize().
>
> Signed-off-by: Ekaterina Tumanova <tuman...@linux.vnet.ibm.com>
> ---
>  block/raw-posix.c | 41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e51293a..38172ca 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -217,11 +217,31 @@ static int raw_normalize_devicepath(const char 
> **filename)
>  }
>  #endif
>  
> +/*
> + * Get logical block size via ioctl. On success return 0. Otherwise -errno.
> + */
> +static int probe_logical_blocksize(int fd, unsigned int *sector_size)
> +{
> +#if defined(BLKSSZGET)
> +#  define SECTOR_SIZE BLKSSZGET
> +#elif defined(DKIOCGETBLOCKSIZE)
> +#  define SECTOR_SIZE DKIOCGETBLOCKSIZE
> +#elif defined(DIOCGSECTORSIZE)
> +#  define SECTOR_SIZE DIOCGSECTORSIZE
> +#else
> +    return -ENOTSUP
> +#endif
> +    if (ioctl(fd, SECTOR_SIZE, sector_size) < 0) {
> +        return -errno;
> +    }
> +    return 0;
> +#undef SECTOR_SIZE
> +}
> +
>  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 +251,12 @@ 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;
> -
> -#ifdef BLKSSZGET
> -    if (ioctl(fd, BLKSSZGET, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> -    }
> -#endif
> -#ifdef DKIOCGETBLOCKSIZE
> -    if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    if (probe_logical_blocksize(fd, &bs->request_alignment) < 0) {
> +        bs->request_alignment = 0;
>      }
> -#endif
> -#ifdef DIOCGSECTORSIZE
> -    if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> -    }
> -#endif
>  #ifdef CONFIG_XFS
>      if (s->is_xfs) {
>          struct dioattr da;

New in v4: this isn't just factoring out code, you also change it!

Before: try BLKSSZGET (if defined), DKIOCGETBLOCKSIZE (if defined),
DIOCGSECTORSIZE (if defined) in order until.  The last one to succeed
wins.  If none succeeds, assume 0.

After: use BLKSSZGET if defined, else try DKIOCGETBLOCKSIZE if defined,
else try DIOCGSECTORSIZE if defined, else assume 0.

Your change may well be an improvement.  But your commit message should
explain *why* it's an improvement.

I guess I'd do a separate patch for that, but since the code patch seems
obvious enough as it is, you may just improve its commit message and
call it a day.