Re: [Qemu-devel] [PATCH v5 2/5] raw-posix: Refactor logical block size detection.

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

On 01/02/2015 02:52 PM, Stefan Hajnoczi wrote:
> On Thu, Dec 18, 2014 at 12:18:01PM +0100, Ekaterina Tumanova wrote:
>> +#if defined(BLKSSZGET)
>> +#  define SECTOR_SIZE BLKSSZGET
>> +#elif defined(DKIOCGETBLOCKSIZE)
>> +#  define SECTOR_SIZE DKIOCGETBLOCKSIZE
>> +#elif defined(DIOCGSECTORSIZE)
>> +#  define SECTOR_SIZE DIOCGSECTORSIZE
>> +#else
>> +    return -ENOTSUP
>> +#endif
>> +    if (ioctl(fd, SECTOR_SIZE, sector_size) < 0) {
>> +        return -errno;
>> +    }
>> +    return 0;
>> +#undef SECTOR_SIZE
>
> Not a reason to respin, but I would have preferred simply moving the old
> code.
>
> I think the new code works because BLKSSZGET is Linux, DKIOCGETBLOCKSIZE
> is Mac OS, and DIOCGSECTORSIZE is FreeBSD.
>
> If there is a host OS where more than one ioctl is available and the
> first one fails then the new code is broken.  The old code didn't use
> #elif so each ioctl had a chance to run.
>

In this case, why should it have a chance to run, if we only use one
result at a time? (Old code overwrites first result with the second)

Plus as far as I understand, in this hypothetical case of 2 ioctls
defined, one will most probably will be a redefinition of another.

p.s. ok to all further comments to patches.

Thanks!
Kate.


> Also, the name SECTOR_SIZE is misleading.  It's not a sector size value
> but the "get sector size" ioctl request code.
>
> Stefan
>