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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

On Tue, Jan 13, 2015 at 01:03:03PM +0300, Ekaterina Tumanova wrote:
> 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.

As code reviewer, I don't know exactly what all supported host OSes do
(Linux, *BSD, Solaris, AIX, etc).

If you leave the control flow unchanged then I'm confident that this
patch doesn't introduce a bug.

If you rewrite the control flow, then the semantics are different but I
can't verify that they are correct in all cases.

Spurious code changes make the life of code reviewers harder.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]