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

[Ben Hutchings]

On Tue, 2016-02-16 at 12:51 -0500, David Miller wrote:
> From: Rainer Weikusat <rweikusat@xxxxxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
> > Signed-off-by: Rainer Weikusat <rweikusat@xxxxxxxxxxxxxxxxxxxxxxx>
> 
> 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: signature.asc
Description: This is a digitally signed message part