Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Markus Armbruster]

Ekaterina Tumanova <address@hidden> writes:

> Move the IOCTL calls that detect logical blocksize from raw_probe_alignment
> into separate function (probe_logical_blocksize).
> Introduce function which detect physical blocksize via IOCTL
> (probe_physical_blocksize).
> Both functions will be used in the next patch.

The first one is used in this patch, actually.

>
> Signed-off-by: Ekaterina Tumanova <address@hidden>

Subject's subsystem is "geometry".  Geometry is your topic.  The
subsystem is what you're patching.  Here, it's "raw-posix" or
"block/raw-posix".  Likewise for the other patches in this series.

Stefan asked you move probe_physical_blocksize() to the patch that uses
it, and I concur.

With that done, I'd write a commit message like

    raw-posix: Factor block size detection out of raw_probe_alignment()

    Put it in new probe_logical_blocksize().
  
> ---
>  block/raw-posix.c | 58 
> +++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 19 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e100ae2..45f1d79 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char 
> **filename)
>  }
>  #endif
>  
> -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd)

Parameter bs is unused, let's drop it.

Suggest function comment

    /**
     * Return logical block size, or zero if we can't figure it out
     */

>  {
> -    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. */
> -    if (bs->sg || !s->needs_alignment) {
> -        bs->request_alignment = 1;
> -        s->buf_align = 1;
> -        return;
> -    }
> +    unsigned int sector_size = 0;

Pointless initialization.

>  
>      /* 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;
> +        return sector_size;
>      }
>  #endif
>  #ifdef DKIOCGETBLOCKSIZE
>      if (ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef DIOCGSECTORSIZE
>      if (ioctl(fd, DIOCGSECTORSIZE, &sector_size) >= 0) {
> -        bs->request_alignment = sector_size;
> +        return sector_size;
>      }
>  #endif
>  #ifdef CONFIG_XFS
>      if (s->is_xfs) {
>          struct dioattr da;
>          if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, &da) >= 0) {
> -            bs->request_alignment = da.d_miniosz;
> +            sector_size = da.d_miniosz;
>              /* The kernel returns wrong information for d_mem */
>              /* s->buf_align = da.d_mem; */

Since you keep the enabled assignments to s->buf_align out of this
function, you should keep out this disabled one, too.

> +            return sector_size;
>          }
>      }
>  #endif
>  
> +    return 0;
> +}
> +
> +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd)

Parameter bs is unused, let's drop it.

> +{
> +    unsigned int blk_size = 0;

Pointless initialization.

> +#ifdef BLKPBSZGET
> +    if (ioctl(fd, BLKPBSZGET, &blk_size) >= 0) {
> +        return blk_size;
> +    }
> +#endif
> +
> +    return 0;
> +}
> +
> +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    char *buf;
> +
> +    /* For /dev/sg devices the alignment is not really used.
> +       With buffered I/O, we don't have any restrictions. */
> +    if (bs->sg || !s->needs_alignment) {
> +        bs->request_alignment = 1;
> +        s->buf_align = 1;
> +        return;
> +    }
> +
> +    s->buf_align = 0;
> +    /* Let's try to use the logical blocksize for the alignment. */
> +    bs->request_alignment = probe_logical_blocksize(bs, fd);
> +
>      /* If we could not get the sizes so far, we can only guess them */
>      if (!s->buf_align) {
>          size_t align;