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

[Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>]

On 11/28/2014 03:52 PM, Markus Armbruster wrote:
> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes:
>
>>>
>>> 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.
>
> Where?  As far as I can see, we return it only after ioctl() succeeded.
>

Sorry,
you're absolutely right. I kept seeing and thinking that I always
returned sector_size variable. ::facepalm::

>>>>
>>>>        /* 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.
>
> Again, we return it only after ioctl() succeeded.
>
>>>> +#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;
>>>
>