linux-kernel - Re: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation O

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 07 Nov 2014 14:21:18 +0000
From:	Roger Willcocks <roger@...mlight.ltd.uk>
To:	Anton Altaparmakov <aia21@....ac.uk>
Cc:	Anand Avati <avati@...ster.org>, linux-arch@...r.kernel.org,
	linux-aio@...ck.org, linux-nfs@...r.kernel.org,
	Volker Lendecke <Volker.Lendecke@...net.de>,
	Theodore Ts'o <tytso@....edu>, Mel Gorman <mgorman@...e.de>,
	"fuse-devel@...ts.sourceforge.net" <fuse-devel@...ts.sourceforge.net>,
	linux-api@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Michael Kerrisk <mtk.manpages@...il.com>,
	Christoph Hellwig <hch@...radead.org>, linux-mm@...ck.org,
	Jeff Moyer <jmoyer@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>, Tejun Heo <tj@...nel.org>,
	linux-fsdevel@...r.kernel.org, ceph-devel@...r.kernel.org,
	Christoph Hellwig <hch@....de>, ocfs2-devel@....oracle.com,
	Milosz Tanski <milosz@...in.com>
Subject: Re: [fuse-devel] [PATCH v5 7/7] add a flag for per-operation
	O_DSYNC semantics


On Fri, 2014-11-07 at 08:43 +0200, Anton Altaparmakov wrote:
> Hi,
> 
> > On 7 Nov 2014, at 07:52, Anand Avati <avati@...ster.org> wrote:
> > On Thu, Nov 6, 2014 at 8:22 PM, Anton Altaparmakov <aia21@....ac.uk> wrote:
> > > On 7 Nov 2014, at 01:46, Jeff Moyer <jmoyer@...hat.com> wrote:
> > > Minor nit, but I'd rather read something that looks like this:
> > >
> > >       if (type == READ && (flags & RWF_NONBLOCK))
> > >               return -EAGAIN;
> > >       else if (type == WRITE && (flags & RWF_DSYNC))
> > >               return -EINVAL;
> > 
> > But your version is less logically efficient for the case where "type == READ" is true and "flags & RWF_NONBLOCK" is false because your version then has to do the "if (type == WRITE" check before discovering it does not need to take that branch either, whilst the original version does not have to do such a test at all.
> > 
> > Seriously?
> 
> Of course seriously.
> 
> > Just focus on the code readability/maintainability which makes the code most easily understood/obvious to a new pair of eyes, and leave such micro-optimizations to the compiler..
> 
> The original version is more readable (IMO) and this is not a micro-optimization.  It is people like you who are responsible for the fact that we need faster and faster computers to cope with the inefficient/poor code being written more and more...
> 

Your original version needs me to know that type can only be either READ
or WRITE (and not, for instance, READONLY or READWRITE or some other
random special case) and it rings alarm bells when I first see it. If
you want to keep the micro optimization, you need an assertion to
acknowledge the potential bug and a comment to make the code obvious:

 +            assert(type == READ || type == WRITE);
 +            if (type == READ) {
 +                    if (flags & RWF_NONBLOCK)
 +                            return -EAGAIN;
 +            } else { /* WRITE */
 +                    if (flags & RWF_DSYNC)
 +                            return -EINVAL;
 +            }

but since what's really happening here is two separate and independent
error checks, Jeff's version is still better, even if it does take an
extra couple of nanoseconds.

Actually I'd probably write:

       if (type == READ && (flags & RWF_NONBLOCK))
              return -EAGAIN;

       if (type == WRITE && (flags & RWF_DSYNC))
              return -EINVAL;

(no 'else' since the code will never be reached if the first test is
true).


-- 
Roger Willcocks <roger@...mlight.ltd.uk>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/