| Message ID | 1474978605-28116-1-git-send-email-zenczykowski@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Maciej Żenczykowski <zenczykowski@gmail.com> Date: Tue, 27 Sep 2016 05:16:45 -0700 > From: Maciej Żenczykowski <maze@google.com> > > Signed-off-by: Maciej Żenczykowski <maze@google.com> I was about to ask you to submit this patch alongside an actual addition of a proc_dointvec_minmax user to the table. But upon second checking there is one, indirectly via addrconf_sysctl_mtu(). I should never have to search and figure things out like that. It should be obvious or explained in the commit message. You must explain in detail what the dependency is, why you have to make this change, how you made it, and why you made it that way. Especially in this case, your empty commit message is really not appropriate. Thanks.
From: David Miller <davem@davemloft.net> Date: Wed, 28 Sep 2016 07:49:01 -0400 (EDT) > From: Maciej Żenczykowski <zenczykowski@gmail.com> > Date: Tue, 27 Sep 2016 05:16:45 -0700 > >> From: Maciej Żenczykowski <maze@google.com> >> >> Signed-off-by: Maciej Żenczykowski <maze@google.com> > > I was about to ask you to submit this patch alongside an actual > addition of a proc_dointvec_minmax user to the table. > > But upon second checking there is one, indirectly via > addrconf_sysctl_mtu(). And actually these cases use a dummy sysctl blob on the stack to provide the min/max values to proc_dointvec_minmax. And they do it _EXACTLY_ because the extra pointers are for the netns pointer and a pointer to the idev. And any user envisioned can do the same thing, use a trampoline like the existing cases do.
From: Maciej Żenczykowski <zenczykowski@gmail.com> Date: Wed, 28 Sep 2016 22:23:10 +0900 > Anyway, enough, I give up, this isn't worth my time, and it's also not > worth your time. > I removed the dependency from the other patches and squashed them all into > 1 to make reviewing easier. Splitting things up is sometimes warranted, but you have to keep together the things that strongly need each other. And new facilities require examples of their use. They really do. I simply can't look at your patch and empty commit message and figure out why you needed to do what you were doing. Any developer should be able to look at a patch being proposed and be able to understand it standing upon on it's own. This means they shouldn't have to know what happened in this discussion thread or that one in order to evaluate and audit the patch properly.
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 2f1f5d439788..11fa1a5564d4 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -6044,8 +6044,14 @@ static int __addrconf_sysctl_register(struct net *net, char *dev_name, for (i = 0; table[i].data; i++) { table[i].data += (char *)p - (char *)&ipv6_devconf; - table[i].extra1 = idev; /* embedded; no ref */ - table[i].extra2 = net; + /* If one of these is already set, then it is not safe to + * overwrite either of them: this makes proc_dointvec_minmax + * usable. + */ + if (!table[i].extra1 && !table[i].extra2) { + table[i].extra1 = idev; /* embedded; no ref */ + table[i].extra2 = net; + } } snprintf(path, sizeof(path), "net/ipv6/conf/%s", dev_name);