| Message ID | 1550242187-29660-6-git-send-email-laoar.shao@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | clean up SOCK_DEBUG() | expand |
On Fri, Feb 15, 2019 at 6:50 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > As SOCK_DEBUG() isn't used any more, we can get ride of it now. > No, we are still using this infrastructure from time to time. I told you I agreed to remove the current (obsolete) TCP call sites, I never suggested to remove SOCK_DEBUG() completely.
On Fri, Feb 15, 2019 at 10:58 PM Eric Dumazet <edumazet@google.com> wrote: > > On Fri, Feb 15, 2019 at 6:50 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > As SOCK_DEBUG() isn't used any more, we can get ride of it now. > > > > No, we are still using this infrastructure from time to time. > > I told you I agreed to remove the current (obsolete) TCP call sites, > I never suggested to remove SOCK_DEBUG() completely. All right, however I'm not persuaded by you. Because SOCK_DEBUG() is not a smart way for debugging, which requires the user code to be modified. We have other more flexible methords. Thanks Yafang
On Fri, Feb 15, 2019 at 7:42 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > On Fri, Feb 15, 2019 at 10:58 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Fri, Feb 15, 2019 at 6:50 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > As SOCK_DEBUG() isn't used any more, we can get ride of it now. > > > > > > > No, we are still using this infrastructure from time to time. > > > > I told you I agreed to remove the current (obsolete) TCP call sites, > > I never suggested to remove SOCK_DEBUG() completely. > > All right, however I'm not persuaded by you. > Because SOCK_DEBUG() is not a smart way for debugging, which requires > the user code to be modified. > We have other more flexible methords. That might be the case, but only if you get enough privileges and skills. I fail to see why we should enforce anything here. It costs nothing having this interface kept. In any case, we do not deprecate features without a Documentation/ABI/obsolete/* file and quarantine.
On Fri, Feb 15, 2019 at 8:26 AM Eric Dumazet <edumazet@google.com> wrote: > > On Fri, Feb 15, 2019 at 6:50 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > As SOCK_DEBUG() isn't used any more, we can get ride of it now. > > > > No, we are still using this infrastructure from time to time. > > I told you I agreed to remove the current (obsolete) TCP call sites, > I never suggested to remove SOCK_DEBUG() completely. Since when do we upstream care about any out-of-tree users? You can always carry a patch to keep it downstream if you want, no one can stop you doing it.
On Fri, Feb 15, 2019 at 10:13 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Fri, Feb 15, 2019 at 8:26 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Fri, Feb 15, 2019 at 6:50 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > > As SOCK_DEBUG() isn't used any more, we can get ride of it now. > > > > > > > No, we are still using this infrastructure from time to time. > > > > I told you I agreed to remove the current (obsolete) TCP call sites, > > I never suggested to remove SOCK_DEBUG() completely. > > Since when do we upstream care about any out-of-tree users? > > You can always carry a patch to keep it downstream if you want, > no one can stop you doing it. Somehow the patch series seems to present things in this way : Eric Dumazet suggested to remove completely the SOCK_DEBUG() interface. I did not. I sometimes am lazy and use SOCK_DEBUG() myself, so I would like we keep it. I know that others are doing the same thing, so I do not feel any shame. Feel free to ignore my feedback.
On Fri, 2019-02-15 at 10:22 -0800, Eric Dumazet wrote: > On Fri, Feb 15, 2019 at 10:13 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Fri, Feb 15, 2019 at 8:26 AM Eric Dumazet <edumazet@google.com> wrote: > > > On Fri, Feb 15, 2019 at 6:50 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > As SOCK_DEBUG() isn't used any more, we can get ride of it now. > > > > > > > > > > No, we are still using this infrastructure from time to time. > > > > > > I told you I agreed to remove the current (obsolete) TCP call sites, > > > I never suggested to remove SOCK_DEBUG() completely. > > > > Since when do we upstream care about any out-of-tree users? > > > > You can always carry a patch to keep it downstream if you want, > > no one can stop you doing it. > > Somehow the patch series seems to present things in this way : > > Eric Dumazet suggested to remove completely the SOCK_DEBUG() interface. Well, you kinda did. It's certainly reasonable to interpret what you wrote as such. On Tue, 2019-02-12 at 18:15 -0800, Eric Dumazet wrote: > Just remove all SOCK_DEBUG() calls, there are leftovers of very ancient times. My suggestion would be to undefine SOCK_DEBUGGING. Something like: --- include/net/sock.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/include/net/sock.h b/include/net/sock.h index 328cb7cb7b0b..7e39bdfa342a 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
On Sat, Feb 16, 2019 at 2:51 AM Joe Perches <joe@perches.com> wrote: > > On Fri, 2019-02-15 at 10:22 -0800, Eric Dumazet wrote: > > On Fri, Feb 15, 2019 at 10:13 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > On Fri, Feb 15, 2019 at 8:26 AM Eric Dumazet <edumazet@google.com> wrote: > > > > On Fri, Feb 15, 2019 at 6:50 AM Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > As SOCK_DEBUG() isn't used any more, we can get ride of it now. > > > > > > > > > > > > > No, we are still using this infrastructure from time to time. > > > > > > > > I told you I agreed to remove the current (obsolete) TCP call sites, > > > > I never suggested to remove SOCK_DEBUG() completely. > > > > > > Since when do we upstream care about any out-of-tree users? > > > > > > You can always carry a patch to keep it downstream if you want, > > > no one can stop you doing it. > > > > Somehow the patch series seems to present things in this way : > > > > Eric Dumazet suggested to remove completely the SOCK_DEBUG() interface. > > Well, you kinda did. > It's certainly reasonable to interpret what you wrote as such. > > On Tue, 2019-02-12 at 18:15 -0800, Eric Dumazet wrote: > > Just remove all SOCK_DEBUG() calls, there are leftovers of very ancient times. > > My suggestion would be to undefine SOCK_DEBUGGING. > This seems like a reasonable trade-off decision. I will change it like this. > Something like: > --- > include/net/sock.h | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/include/net/sock.h b/include/net/sock.h > index 328cb7cb7b0b..7e39bdfa342a 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 > > Thanks Yafang
diff --git a/include/net/sock.h b/include/net/sock.h index 6679f3c..4c6b599 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -74,25 +74,6 @@ #include <net/smc.h> #include <net/l3mdev.h> -/* - * This structure really needs to be cleaned up. - * Most of it is for TCP, and not used by any of - * the other protocols. - */ - -/* Define this to get the SOCK_DBG debugging facility. */ -#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) -#else -/* Validate arguments and do nothing */ -static inline __printf(2, 3) -void SOCK_DEBUG(const struct sock *sk, const char *msg, ...) -{ -} -#endif - /* This is the per-socket lock. The spinlock provides a synchronization * between user contexts and software interrupt processing, whereas the * mini-semaphore synchronizes multiple users amongst themselves. diff --git a/net/core/sock.c b/net/core/sock.c index 71ded4d..a980ff3 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 doesn't take effect now, + * but we need to keep it for backward compatibility. + */ if (val && !capable(CAP_NET_ADMIN)) ret = -EACCES; else
As SOCK_DEBUG() isn't used any more, we can get ride of it now. Regarding the SO_DEBUG in sock_{s,g}etsockopt, I think it is better to keep as-is, because if we return an errno to tell the application that this optname isn't supported, it may break the application. The application still can use this option but don't take any effect from now on. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- include/net/sock.h | 19 ------------------- net/core/sock.c | 3 +++ 2 files changed, 3 insertions(+), 19 deletions(-)