Messages in this thread | | | From | Willem de Bruijn <> | Date | Mon, 7 Mar 2022 11:03:56 -0500 | Subject | Re: [PATCH] SO_ZEROCOPY should rather return -ENOPROTOOPT |
| |
On Sun, Mar 6, 2022 at 2:22 PM Samuel Thibault <samuel.thibault@labri.fr> wrote: > > Hello, > > Willem de Bruijn, le mar. 01 mars 2022 10:21:41 -0500, a ecrit: > > > > > > > @@ -1377,9 +1377,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > > > > > > > if (!(sk_is_tcp(sk) || > > > > > > > (sk->sk_type == SOCK_DGRAM && > > > > > > > sk->sk_protocol == IPPROTO_UDP))) > > > > > > > - ret = -ENOTSUPP; > > > > > > > + ret = -ENOPROTOOPT; > > > > > > > } else if (sk->sk_family != PF_RDS) { > > > > > > > - ret = -ENOTSUPP; > > > > > > > + ret = -ENOPROTOOPT; > > > > > > > } > > > > > > > if (!ret) { > > > > > > > if (val < 0 || val > 1) > > > > > > > > > > > > That should have been a public error code. Perhaps rather EOPNOTSUPP. > > > > > > > > > > > > The problem with a change now is that it will confuse existing > > > > > > applications that check for -524 (ENOTSUPP). > > > > > > > > > > They were not supposed to hardcord -524... > > > > > > > > > > Actually, they already had to check against EOPNOTSUPP to support older > > > > > kernels, so EOPNOTSUPP is not supposed to pose a problem. > > > > > > > > Which older kernels returned EOPNOTSUPP on SO_ZEROCOPY? > > > > > > Sorry, bad copy/paste, I meant ENOPROTOOPT. > > > > Same point though, right? These are not legacy concerns, but specific > > to applications written to SO_ZEROCOPY. > > > > I expect that most will just ignore the exact error code and will work > > with either. > > Ok, so, is this an Acked-by: you? :)
I did not touch this code on purpose, due to the small risk of legacy users that expect 524.
If you think the benefit outweighs the risk --the same conclusion reached in the commits I mentioned, 2230a7ef5198 ("drop_monitor: Use correct error code") and commit 4a5cdc604b9c ("net/tls: Fix return values to avoid ENOTSUPP")-- then I can support that.
But like those, I think the correct error code then is EOPNOTSUPP. And the commit message would be stronger by explicitly referencing that prior art, and the fact that those changes did not seem to cause problems.
| |