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

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

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