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
pgppw9DjYZtY5.pgp
Description: PGP signature
