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