On Fri, May 22, 2020 at 11:36:23AM -0300, Marcelo Ricardo Leitner wrote: > It's subjective, yes, but we hardly have patches over 5k lines. > In the case here, as changing the functions also requires changing > their call later on the file, it helps to be able to check that is was > properly updated. Ditto for chained functions. > > For example, I can spot things like this easier (from > [PATCH 26/49] sctp: pass a kernel pointer to sctp_setsockopt_auth_key) > > @@ -3646,7 +3641,6 @@ static int sctp_setsockopt_auth_key(struct sock *sk, > } > > out: > - kzfree(authkey); > return ret; > } > ... > @@ -4771,7 +4765,10 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname, > } > > release_sock(sk); > - kfree(kopt); > + if (optname == SCTP_AUTH_KEY) > + kzfree(kopt); > + else > + kfree(kopt); > > out_nounlock: > return retval; > > these are 1k lines apart. > > Yet, your implementation around this is better: > > @@ -3733,7 +3624,7 @@ static int sctp_setsockopt_auth_key(struct sock *sk, > } > > out: > - kzfree(authkey); > + memset(authkey, 0, optlen); > return ret; > } > > so that sctp_setsockopt() doesn't have to handle it specially. Actually that implementation is wrong, if you want to move to a plain kfree it would have to be a memzero_explicit. > What if you two work on a joint patchset for this? The proposals are > quite close. The differences around the setsockopt handling are > minimal already. It is basically variable naming, indentation and one > or another small change like: I don't really want to waste too much time on this, as what I really need is to get the kernel_setsockopt removal series in ASAP. I'm happy to respin this once or twice with clear maintainer guidance (like the memzero_explicit), but I have no idea what you even meant with your other example or naming. Tell me what exact changes you want, and I can do a quick spin, but I don't really want a huge open ended discussion on how to paint the bikeshed.. Alternatively I'll also happily only do a partial conversion for what I need for the kernel_setsockopt removal and let you and Dave decided what you guys prefer for the rest.
