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

[Markus Armbruster]

Ekaterina Tumanova <address@hidden> 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.

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