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

[Ekaterina Tumanova]

> 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.

If I do not initialize the sector_size, and the ioctl fails,
I will return garbage as a blocksize to the caller.

> >       /* 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.

Same here.

> > +#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;