| Message ID | 1550413592-7877-3-git-send-email-laoar.shao@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | clean up SOCK_DEBUG() | expand |
From: Yafang Shao <laoar.shao@gmail.com> Date: Sun, 17 Feb 2019 22:26:32 +0800 > SOCK_DEBUG() is a old facility for debugging. > If the user want to use it for debugging, the user must modify the > application first, that doesn't seem like a good way. > Now we have more powerful facilities, i.e. bpf or tracepoint, for this kind > of debugging purpose. > So we'd better disable it by default. > The reason why I don't remove it comepletely is that someone may still > would like to use it for debugging. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > Suggested-by: Joe Perches <joe@perches.com> Sorry, I'm not applying this. You are forcing everyone who wants to use this to add a curstom local source code change into their build. I'm OK with patch #1, and Eric suggested it, so if you want to submit that separately that's fine.
On Sat, 2019-02-23 at 13:21 -0800, David Miller wrote: > From: Yafang Shao <laoar.shao@gmail.com> > Date: Sun, 17 Feb 2019 22:26:32 +0800 > > > SOCK_DEBUG() is a old facility for debugging. > > If the user want to use it for debugging, the user must modify the > > application first, that doesn't seem like a good way. > > Now we have more powerful facilities, i.e. bpf or tracepoint, for this kind > > of debugging purpose. > > So we'd better disable it by default. > > The reason why I don't remove it comepletely is that someone may still > > would like to use it for debugging. > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > Suggested-by: Joe Perches <joe@perches.com> > > Sorry, I'm not applying this. > > You are forcing everyone who wants to use this to add a curstom local > source code change into their build. That may not actually be a bad thing. It could become a CONFIG bit too. I'm not sure how often it's actually used. Outside of the likely to be removed tcp uses, it's currently used only in appletalk/ddp, x25, and the 1 use in dccp. All of the existing uses could likely be removed without much notice by anyone.
On Sat, Feb 23, 2019 at 1:21 PM David Miller <davem@davemloft.net> wrote: > > You are forcing everyone who wants to use this to add a curstom local > source code change into their build. What's wrong with this? People carry custom changes in anyway, do we really need to care about all the downstream changes? If yes, can I just add some debugging prink's that no one is going to use except me? My argument would be you can't force me to add a custom local source code change. Can we just play this fairly instead of just for any special people? Thanks!
On Sat, Feb 23, 2019 at 1:21 PM David Miller <davem@davemloft.net> wrote: > > From: Yafang Shao <laoar.shao@gmail.com> > Date: Sun, 17 Feb 2019 22:26:32 +0800 > > The reason why I don't remove it comepletely is that someone may still > > would like to use it for debugging. Please remove it completely. The rule here is we upstream only care about in-tree users, any out-of-tree user is beyond what we care. They either need to push their code that uses this to upstream, or have to carry a revert downstream. People carry downstream changes all the time, this one isn't an exception. Let's respect rules. Thanks.
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Sat, 23 Feb 2019 17:11:08 -0800 > On Sat, Feb 23, 2019 at 1:21 PM David Miller <davem@davemloft.net> wrote: >> >> You are forcing everyone who wants to use this to add a curstom local >> source code change into their build. > > What's wrong with this? People carry custom changes in anyway, > do we really need to care about all the downstream changes? No way am I allowing this, sorry. Either it's something in the tree that people can use or it's a crap hack. When we see stuff like this in a driver we ask the driver author to remove it.
On Sat, Feb 23, 2019 at 6:31 PM David Miller <davem@davemloft.net> wrote: > > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Sat, 23 Feb 2019 17:11:08 -0800 > > > On Sat, Feb 23, 2019 at 1:21 PM David Miller <davem@davemloft.net> wrote: > >> > >> You are forcing everyone who wants to use this to add a curstom local > >> source code change into their build. > > > > What's wrong with this? People carry custom changes in anyway, > > do we really need to care about all the downstream changes? > > No way am I allowing this, sorry. > > Either it's something in the tree that people can use or it's > a crap hack. > > When we see stuff like this in a driver we ask the driver author to > remove it. Just to clarify, I have been suggesting to completely remove this unused macro, never suggest to just undefine it in-tree. There is no reason to keep it in-tree, whether defined or undefined, just for downstream users. Thanks.
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 25 Feb 2019 14:00:09 -0800 > Just to clarify, I have been suggesting to completely remove > this unused macro, never suggest to just undefine it in-tree. > > There is no reason to keep it in-tree, whether defined or undefined, > just for downstream users. And this is where you and I fundamentally disagree.
On Mon, Feb 25, 2019 at 2:29 PM David Miller <davem@davemloft.net> wrote: > > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Mon, 25 Feb 2019 14:00:09 -0800 > > > Just to clarify, I have been suggesting to completely remove > > this unused macro, never suggest to just undefine it in-tree. > > > > There is no reason to keep it in-tree, whether defined or undefined, > > just for downstream users. > > And this is where you and I fundamentally disagree. So you agree that I can add debugging printk's only for my own use? I can claim that I only use them downstream and you can't force me to carry local changes? If not, what is your criteria for accepting debugging printk's? Whose can be accepted and whose can't? Please be specific, and ideally make it a formal doc in netdev-FAQ.txt. Thanks!
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 25 Feb 2019 14:58:36 -0800 > So you agree that I can add debugging printk's only for my own use? Let's put it this way. If I ignore the fact that some of the data centers with the furtest reach on the entire planet use something that is in the tree already, then I am being unreasonable and insensitive. This means no adding, but keeping things we already have. Don't try to weasel word what I'm trying to say here Cong. Have a nice day.
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Mon, 25 Feb 2019 14:58:36 -0800 > Please be specific, and ideally make it a formal doc in netdev-FAQ.txt. And this isn't happening either Cong, I reserve the right to apply my judgment as networking maintainer on a case by case basis as I so see fit.
(Cc'ing LKML) On Mon, Feb 25, 2019 at 3:11 PM David Miller <davem@davemloft.net> wrote: > > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Mon, 25 Feb 2019 14:58:36 -0800 > > > Please be specific, and ideally make it a formal doc in netdev-FAQ.txt. > > And this isn't happening either Cong, I reserve the right to apply my > judgment as networking maintainer on a case by case basis as I so see > fit. Thanks for rephrasing non-transparency in such a wonderful way.
On Tue, Feb 26, 2019 at 6:58 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Feb 25, 2019 at 2:29 PM David Miller <davem@davemloft.net> wrote: > > > > From: Cong Wang <xiyou.wangcong@gmail.com> > > Date: Mon, 25 Feb 2019 14:00:09 -0800 > > > > > Just to clarify, I have been suggesting to completely remove > > > this unused macro, never suggest to just undefine it in-tree. > > > > > > There is no reason to keep it in-tree, whether defined or undefined, > > > just for downstream users. > > > > And this is where you and I fundamentally disagree. > > So you agree that I can add debugging printk's only for my own use? > I can claim that I only use them downstream and you can't force me > to carry local changes? > > If not, what is your criteria for accepting debugging printk's? Whose > can be accepted and whose can't? > > Please be specific, and ideally make it a formal doc in netdev-FAQ.txt. > Per my personal view, I agree with you that we should remove it completely. Clean up such kind of legacy code can make the kernel more clean. Thanks Yafang
diff --git a/include/net/sock.h b/include/net/sock.h index 6679f3c..444e92f 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -81,14 +81,17 @@ */ /* Define this to get the SOCK_DBG debugging facility. */ -#define SOCK_DEBUGGING +/* #define SOCK_DEBUGGING */ #ifdef SOCK_DEBUGGING -#define SOCK_DEBUG(sk, msg...) do { if ((sk) && sock_flag((sk), SOCK_DBG)) \ - printk(KERN_DEBUG msg); } while (0) +#define SOCK_DEBUG(sk, fmt, ...) \ +do { \ + if ((sk) && sock_flag((sk), SOCK_DBG)) \ + printk(KERN_DEBUG fmt, ##__VA_ARGS__); \ +} while (0) #else /* Validate arguments and do nothing */ -static inline __printf(2, 3) -void SOCK_DEBUG(const struct sock *sk, const char *msg, ...) +__printf(2, 3) +static inline void SOCK_DEBUG(const struct sock *sk, const char *fmt, ...) { } #endif diff --git a/net/core/sock.c b/net/core/sock.c index 71ded4d..7c15835 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -753,6 +753,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname, switch (optname) { case SO_DEBUG: + /* This option takes effect only when SOCK_DEBUGGING + * is defined. + */ if (val && !capable(CAP_NET_ADMIN)) ret = -EACCES; else
SOCK_DEBUG() is a old facility for debugging. If the user want to use it for debugging, the user must modify the application first, that doesn't seem like a good way. Now we have more powerful facilities, i.e. bpf or tracepoint, for this kind of debugging purpose. So we'd better disable it by default. The reason why I don't remove it comepletely is that someone may still would like to use it for debugging. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Joe Perches <joe@perches.com> --- include/net/sock.h | 13 ++++++++----- net/core/sock.c | 3 +++ 2 files changed, 11 insertions(+), 5 deletions(-)