[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 2/5] raw-posix: Refactor logical block size d
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v5 2/5] raw-posix: Refactor logical block size detection. |
Date: |
Tue, 13 Jan 2015 15:24:38 +0000 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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
pgpQPRR56haPB.pgp
Description: PGP signature