Ekaterina Tumanova <tuman...@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.
>>>
>>> /* Try a few ioctls to get the right size */
>>> - bs->request_alignment = 0;
>>> - s->buf_align = 0;
>>> -
>>> #ifdef BLKSSZGET
>>> if (ioctl(fd, BLKSSZGET, §or_size) >= 0) {
>>> - bs->request_alignment = sector_size;
>>> + return sector_size;
>>> }
>>> #endif
>>> #ifdef DKIOCGETBLOCKSIZE
>>> if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) {
>>> - bs->request_alignment = sector_size;
>>> + return sector_size;
>>> }
>>> #endif
>>> #ifdef DIOCGSECTORSIZE
>>> if (ioctl(fd, DIOCGSECTORSIZE, §or_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;
>>
