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

On Tue, 2016-02-16 at 12:51 -0500, David Miller wrote:

From: Rainer Weikusat <redacted>
Date: Mon, 08 Feb 2016 18:47:19 +0000

quoted

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 <redacted>
Signed-off-by: Rainer Weikusat <redacted>

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.