Re: [PATCH] af_unix: Don't set err in unix_stream_read_generic unless there was an error

[Ben Hutchings <ben@decadent.org.uk>]

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

On Tue, 2016-02-16 at 12:51 -0500, David Miller wrote:
> From: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> Date: Mon, 08 Feb 2016 18:47:19 +0000
> 
> > The present unix_stream_read_generic contains various code sequences of
> > the form
> > 
> > err = -EDISASTER;
> > if ()
> >       goto out;
> > 
> > This has the unfortunate side effect of possibly causing the error code
> > to bleed through to the final
> > 
> > out:
> >       return copied ? : err;
> > 
> > and then to be wrongly returned if no data was copied because the caller
> > didn't supply a data buffer, as demonstrated by the program available at
> > 
> > http://pad.lv/1540731
> > 
> > Change it such that err is only set if an error condition was detected.
> > 
> > Fixes: 3822b5c2fc62 ("af_unix: Revert 'lock_interruptible' in stream receive code")
> > Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
> > Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> 
> Applied, thanks Rainer.
> 
> And BTW I disagree with some of the feedback I saw in these threads
> about "if (x) goto out;" being unreadable and that it should be avoided.

That's not what I said.

> That's completely wrong.
> 
> Fact is, we've all been reading code of that form for multiple decades.
> So it's the style we are _MOST_ familiar with, and it is therefore the
> style that is the easiest and clearest for kernel developers to understand.
[...]

I agree that 'if (err) goto cleanup;' is widely used and is generally
understandable (though more creative uses of goto are often not).

My objection was to 'err = -EFOO; if (cond) goto cleanup;'.  That is definitely not clear and it hides mistakes like this.

Ben.

-- 
Ben Hutchings
Lowery's Law:
             If it jams, force it. If it breaks, it needed replacing anyway.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]