[PATCH] net: diag: Tell user if support for destroying TCP sockets is not enabled

[PATCH] net: diag: Tell user if support for destroying TCP sockets is not enabled

[David Ahern]

Commit c1e64e298b8c added support for destroying TCP sockets but it is
wrapped in a config option. If the option is not enabled the user is given
no feedback and ss for example just exits 0 which is not a friendly UI:

$ ss -4  state established sport = :22
Netid  Recv-Q Send-Q      Local Address:Port     Peer Address:Port
tcp    0      0           10.1.1.2:ssh           192.168.2.50:47438

$ ss -4  -K state established sport = :22 dport = :47438
Netid  Recv-Q Send-Q      Local Address:Port     Peer Address:Port
(nothing else in the output and the connection lives on).

Fix by returning an error to the user if the config option is not
enabled:

$ ss -4 -K state established sport = :22 dport = :47450
Netid  Recv-Q Send-Q      Local Address:Port     Peer Address:Port
SOCK_DESTROY answers: Operation not supported

Fixes: c1e64e298b8c ("net: diag: Support destroying TCP sockets.")
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/tcp_diag.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index 4d610934fb39..99590423d468 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -60,6 +60,12 @@ static int tcp_diag_destroy(struct sk_buff *in_skb,
 
 	return sock_diag_destroy(sk, ECONNABORTED);
 }
+#else
+static int tcp_diag_destroy(struct sk_buff *in_skb,
+			    const struct inet_diag_req_v2 *req)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 static const struct inet_diag_handler tcp_diag_handler = {
@@ -68,9 +74,7 @@ static const struct inet_diag_handler tcp_diag_handler = {
 	.idiag_get_info	 = tcp_diag_get_info,
 	.idiag_type	 = IPPROTO_TCP,
 	.idiag_info_size = sizeof(struct tcp_info),
-#ifdef CONFIG_INET_DIAG_DESTROY
 	.destroy	 = tcp_diag_destroy,
-#endif
 };
 
 static int __init tcp_diag_init(void)
-- 
2.1.4

[PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

Silent failures are not friendly to the user. If a command is
not supported tell the user about it.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 misc/ss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/ss.c b/misc/ss.c
index 23fff19d9199..bd7214c85938 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2264,7 +2264,7 @@ static int show_one_inet_sock(const struct sockaddr_nl *addr,
 	if (!(diag_arg->f->families & (1 << r->idiag_family)))
 		return 0;
 	if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
-		if (errno == EOPNOTSUPP || errno == ENOENT) {
+		if (errno == ENOENT) {
 			/* Socket can't be closed, or is already closed. */
 			return 0;
 		} else {
-- 
2.1.4

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Tue, May 17, 2016 at 8:53 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> @@ -2264,7 +2264,7 @@ static int show_one_inet_sock(const struct sockaddr_nl *addr,
>         if (!(diag_arg->f->families & (1 << r->idiag_family)))
>                 return 0;
>         if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
> -               if (errno == EOPNOTSUPP || errno == ENOENT) {
> +               if (errno == ENOENT) {
>                         /* Socket can't be closed, or is already closed. */
>                         return 0;
>                 } else {

I don't think you can do this without breaking the functionality of -K.

The else branch will cause show_one_inet_sock to return -1, which will
cause rtnl_dump_filter to abort and not close any other sockets that
the user requested killing. That's incorrect, because getting
EOPNOTSUPP on one socket doesn't necessarily mean we'll get EOPNOTSUPP
on any future sockets in the same dump.

For example, EOPNOTSUPP can just mean "this socket can't be closed
because it's a timewait or NEW_SYN_RECV socket". In hindsight it might
have been better to return EBADFD in those cases, but that still
doesn't solve the UI problem. If the user does something like "ss -K
dport = :443", the user would expect the command to kill all TCP
sockets and not just abort if there happens to be a UDP socket to port
443 (which can't be closed because UDP doesn't currently implement
SOCK_DESTROY).

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/16/16 7:01 PM, Lorenzo Colitti wrote:
> On Tue, May 17, 2016 at 8:53 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> @@ -2264,7 +2264,7 @@ static int show_one_inet_sock(const struct sockaddr_nl *addr,
>>         if (!(diag_arg->f->families & (1 << r->idiag_family)))
>>                 return 0;
>>         if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
>> -               if (errno == EOPNOTSUPP || errno == ENOENT) {
>> +               if (errno == ENOENT) {
>>                         /* Socket can't be closed, or is already closed. */
>>                         return 0;
>>                 } else {
>
> I don't think you can do this without breaking the functionality of -K.
>
> The else branch will cause show_one_inet_sock to return -1, which will
> cause rtnl_dump_filter to abort and not close any other sockets that
> the user requested killing. That's incorrect, because getting
> EOPNOTSUPP on one socket doesn't necessarily mean we'll get EOPNOTSUPP
> on any future sockets in the same dump.
>
> For example, EOPNOTSUPP can just mean "this socket can't be closed
> because it's a timewait or NEW_SYN_RECV socket". In hindsight it might
> have been better to return EBADFD in those cases, but that still
> doesn't solve the UI problem. If the user does something like "ss -K
> dport = :443", the user would expect the command to kill all TCP
> sockets and not just abort if there happens to be a UDP socket to port
> 443 (which can't be closed because UDP doesn't currently implement
> SOCK_DESTROY).
>

Silently doing nothing is just as bad - or worse. I was running in 
circles trying to figure out why nothing was happening and ss was 
exiting 0.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Tue, May 17, 2016 at 10:14 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>>
>> For example, EOPNOTSUPP can just mean "this socket can't be closed
>> because it's a timewait or NEW_SYN_RECV socket". In hindsight it might
>> have been better to return EBADFD in those cases, but that still
>> doesn't solve the UI problem. If the user does something like "ss -K
>> dport = :443", the user would expect the command to kill all TCP
>> sockets and not just abort if there happens to be a UDP socket to port
>> 443 (which can't be closed because UDP doesn't currently implement
>> SOCK_DESTROY).
>
>
> Silently doing nothing is just as bad - or worse. I was running in circles trying to figure out why nothing was happening and ss was exiting 0.


At least that's documented to be the case in the man page.

On the other hand, if your patch is applied, there will be no way to
close more than one socket if one of them returns EOPNOTSUPP. On a
busy server where things go into TIME_WAIT all the time, you might
never be able to close all sockets.

If you want to inform the user, then you could do so via the return
value of ss - e.g., return 0 if at least one socket was printed and
closed, or 1 otherwise.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/16/16 7:20 PM, Lorenzo Colitti wrote:
> On Tue, May 17, 2016 at 10:14 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>>>
>>> For example, EOPNOTSUPP can just mean "this socket can't be closed
>>> because it's a timewait or NEW_SYN_RECV socket". In hindsight it might
>>> have been better to return EBADFD in those cases, but that still
>>> doesn't solve the UI problem. If the user does something like "ss -K
>>> dport = :443", the user would expect the command to kill all TCP
>>> sockets and not just abort if there happens to be a UDP socket to port
>>> 443 (which can't be closed because UDP doesn't currently implement
>>> SOCK_DESTROY).
>>
>>
>> Silently doing nothing is just as bad - or worse. I was running in circles trying to figure out why nothing was happening and ss was exiting 0.
>
>
> At least that's documented to be the case in the man page.
>
> On the other hand, if your patch is applied, there will be no way to
> close more than one socket if one of them returns EOPNOTSUPP. On a
> busy server where things go into TIME_WAIT all the time, you might
> never be able to close all sockets.
>
> If you want to inform the user, then you could do so via the return
> value of ss - e.g., return 0 if at least one socket was printed and
> closed, or 1 otherwise.
>

code is not setup to handle that. Only option seems to be at least dump 
an error message, but the message can not relate any of the specifics 
about the filter. So something like this though it dumps the message per 
socket matched by the filter. Could throttle it to once.

diff --git a/misc/ss.c b/misc/ss.c
index 23fff19d9199..1925c6fd9c36 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2264,8 +2264,12 @@ static int show_one_inet_sock(const struct 
sockaddr_nl *addr,
         if (!(diag_arg->f->families & (1 << r->idiag_family)))
                 return 0;
         if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
-               if (errno == EOPNOTSUPP || errno == ENOENT) {
-                       /* Socket can't be closed, or is already closed. */
+               if (errno == ENOENT) {
+                       /* socket is already closed. */
+                       return 0;
+               /* Socket can't be closed OR config is not enabled */
+               } else if (errno == EOPNOTSUPP) {
+                       perror("SOCK_DESTROY answers");
                         return 0;
                 } else {
                         perror("SOCK_DESTROY answers");

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Tue, May 17, 2016 at 10:52 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> code is not setup to handle that. Only option seems to be at least dump an
> error message, but the message can not relate any of the specifics about the
> filter. So something like this though it dumps the message per socket
> matched by the filter. Could throttle it to once.
> [...]
>         if (diag_arg->f->kill && kill_inet_sock(h, arg) != 0) {
> -               if (errno == EOPNOTSUPP || errno == ENOENT) {
> -                       /* Socket can't be closed, or is already closed. */
> +               if (errno == ENOENT) {
> +                       /* socket is already closed. */
> +                       return 0;
> +               /* Socket can't be closed OR config is not enabled */
> +               } else if (errno == EOPNOTSUPP) {
> +                       perror("SOCK_DESTROY answers");

The reason the code was written like that is that I didn't want to
print one error message for every socket that can't be closed - such
as TIME_WAIT sockets or UDP sockets.

Given that the filter can specify a number of sockets, some of which
can and some of which can't be closed, and that whether a given socket
can be closed is only known at the time we attempt to close it, there
is a choice between two bad outcomes:

1. Users try to use "ss -K" with a kernel that doesn't support it, and
get confused about why it does nothing and doesn't print an error
message.
2. Users use "ss -K" with a kernel that does support it, and get
irritated by seeing one error message per TCP_TIME_WAIT socket, UDP
socket, etc.

Personally I think it's more important to avoid #2 than #1, because #1
is one time (only if you're compiling your own kernel), but #2 is
forever. Also, I think it's consistent with other behaviours in ss -
for example, if the kernel doesn't support SOCK_DIAG for UDP, you just
get nothing back if you run "ss -u".

That said, I'm not the maintainer of this code. Stephen, any thoughts?

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/16/16 8:04 PM, Lorenzo Colitti wrote:
> Given that the filter can specify a number of sockets, some of which
> can and some of which can't be closed, and that whether a given socket
> can be closed is only known at the time we attempt to close it, there
> is a choice between two bad outcomes:
>
> 1. Users try to use "ss -K" with a kernel that doesn't support it, and
> get confused about why it does nothing and doesn't print an error
> message.
> 2. Users use "ss -K" with a kernel that does support it, and get
> irritated by seeing one error message per TCP_TIME_WAIT socket, UDP
> socket, etc.

As I mentioned we can print the unsupported once or per socket matched 
and with the socket params. e.g.,

+               } else if (errno == EOPNOTSUPP) {
+                       printf("Operation not supported for:\n");
+                       inet_show_sock(h, diag_arg->f, diag_arg->protocol);

Actively suppressing all error messages is just wrong. I get the 
flooding issue so I'm fine with just printing it once.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Tue, May 17, 2016 at 11:24 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> As I mentioned we can print the unsupported once or per socket matched and
> with the socket params. e.g.,
>
> +               } else if (errno == EOPNOTSUPP) {
> +                       printf("Operation not supported for:\n");
> +                       inet_show_sock(h, diag_arg->f, diag_arg->protocol);
>
> Actively suppressing all error messages is just wrong. I get the flooding
> issue so I'm fine with just printing it once.

I disagree, but then I'm the one who wrote it in the first place, so
you wouldn't expect me to agree. :-) Let's see what Stephen says.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[subashab]

On 2016-05-16 20:29, Lorenzo Colitti wrote:
> On Tue, May 17, 2016 at 11:24 AM, David Ahern <dsa@cumulusnetworks.com> 
> wrote:
>> As I mentioned we can print the unsupported once or per socket matched 
>> and
>> with the socket params. e.g.,
>> 
>> +               } else if (errno == EOPNOTSUPP) {
>> +                       printf("Operation not supported for:\n");
>> +                       inet_show_sock(h, diag_arg->f, 
>> diag_arg->protocol);
>> 
>> Actively suppressing all error messages is just wrong. I get the 
>> flooding
>> issue so I'm fine with just printing it once.
> 
> I disagree, but then I'm the one who wrote it in the first place, so
> you wouldn't expect me to agree. :-) Let's see what Stephen says.

Hi Lorenzo

Would it be acceptable to have a separate column which displays the 
result of the sock destroy operation per socket.
State    ... Killed
ESTAB         Y
TIME_WAIT     N

If it is not supported from kernel, maybe print U (unsupported) for 
this.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Stephen Hemminger]

On Tue, 17 May 2016 12:35:53 -0600
subashab@codeaurora.org wrote:

> On 2016-05-16 20:29, Lorenzo Colitti wrote:
> > On Tue, May 17, 2016 at 11:24 AM, David Ahern <dsa@cumulusnetworks.com> 
> > wrote:
> >> As I mentioned we can print the unsupported once or per socket matched 
> >> and
> >> with the socket params. e.g.,
> >> 
> >> +               } else if (errno == EOPNOTSUPP) {
> >> +                       printf("Operation not supported for:\n");
> >> +                       inet_show_sock(h, diag_arg->f, 
> >> diag_arg->protocol);
> >> 
> >> Actively suppressing all error messages is just wrong. I get the 
> >> flooding
> >> issue so I'm fine with just printing it once.
> > 
> > I disagree, but then I'm the one who wrote it in the first place, so
> > you wouldn't expect me to agree. :-) Let's see what Stephen says.
> 
> Hi Lorenzo
> 
> Would it be acceptable to have a separate column which displays the 
> result of the sock destroy operation per socket.
> State    ... Killed
> ESTAB         Y
> TIME_WAIT     N
> 
> If it is not supported from kernel, maybe print U (unsupported) for 
> this.

When you guys come to a conclusion, then I will review the result.
Right now neither solution looks good.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Wed, May 18, 2016 at 3:35 AM, <subashab@codeaurora.org> wrote:
> Would it be acceptable to have a separate column which displays the result of the sock destroy operation per socket.
> State    ... Killed
> ESTAB         Y
> TIME_WAIT     N

Fine by me, but... what problem are we trying to address? People who
compile their own kernels and don't turn CONFIG_INET_DIAG_DESTROY, and
then are confused why it doesn't work? Seems like we could fix that by
turning CONFIG_INET_DIAG_DESTROY on by default. CCing the people who
commented on the original SOCK_DESTROY patch to see if they have
opinions.

> If it is not supported from kernel, maybe print U (unsupported) for this.

In current code there is no way to distinguish U from N because in
both cases the error will be EOPNOTSUPP. It's certainly possible to
change SOCK_DESTROY to return something else (e.g., EBADFD) to
indicate "kernel supports closing this type of socket, but it can't be
closed due to the state it's in". In hindsight, perhaps I should have
done that from the start.

Regardless, we still have the problem of what to do if the user says
"ss -K dport = :443" and we encounter a UDP socket connected to port
443. Options:

1. Silently skip. if the tool prints something, it means it closed it.
2. Abort with an error message.
3. Skip the socket and print an error every time this happens.
4. Skip the socket and print an error the first time this happens.

Personally I still think #1 is the best option.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/18/16 6:55 PM, Lorenzo Colitti wrote:
> On Wed, May 18, 2016 at 3:35 AM, <subashab@codeaurora.org> wrote:
>> Would it be acceptable to have a separate column which displays the result of the sock destroy operation per socket.
>> State    ... Killed
>> ESTAB         Y
>> TIME_WAIT     N
>
> Fine by me, but... what problem are we trying to address? People who
> compile their own kernels and don't turn CONFIG_INET_DIAG_DESTROY, and
> then are confused why it doesn't work? Seems like we could fix that by
> turning CONFIG_INET_DIAG_DESTROY on by default. CCing the people who
> commented on the original SOCK_DESTROY patch to see if they have
> opinions.

The problem is proper feedback to a user. If the kernel supports an 
action but the action is not enabled the user should get some message to 
that fact. Doing nothing and exiting 0 is just wrong.

>
>> If it is not supported from kernel, maybe print U (unsupported) for this.
>
> In current code there is no way to distinguish U from N because in
> both cases the error will be EOPNOTSUPP. It's certainly possible to
> change SOCK_DESTROY to return something else (e.g., EBADFD) to
> indicate "kernel supports closing this type of socket, but it can't be
> closed due to the state it's in". In hindsight, perhaps I should have
> done that from the start.
>
> Regardless, we still have the problem of what to do if the user says
> "ss -K dport = :443" and we encounter a UDP socket connected to port
> 443. Options:
>
> 1. Silently skip. if the tool prints something, it means it closed it.
> 2. Abort with an error message.
> 3. Skip the socket and print an error every time this happens.
> 4. Skip the socket and print an error the first time this happens.
>
> Personally I still think #1 is the best option.

5. Print an error the first time and a summary at the end.

If the filter matches N sockets and all N fail with UNSUPPORTED give the 
user a message saying that all failed due to unsupported error which 
could mean the CONFIG option is not enabled or it could mean the sockets 
can not be forced closed.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Eric Dumazet]

On Wed, 2016-05-18 at 21:02 -0600, David Ahern wrote:
> On 5/18/16 6:55 PM, Lorenzo Colitti wrote:
> > On Wed, May 18, 2016 at 3:35 AM, <subashab@codeaurora.org> wrote:
> >> Would it be acceptable to have a separate column which displays the result of the sock destroy operation per socket.
> >> State    ... Killed
> >> ESTAB         Y
> >> TIME_WAIT     N
> >
> > Fine by me, but... what problem are we trying to address? People who
> > compile their own kernels and don't turn CONFIG_INET_DIAG_DESTROY, and
> > then are confused why it doesn't work? Seems like we could fix that by
> > turning CONFIG_INET_DIAG_DESTROY on by default. CCing the people who
> > commented on the original SOCK_DESTROY patch to see if they have
> > opinions.
> 
> The problem is proper feedback to a user. If the kernel supports an 
> action but the action is not enabled the user should get some message to 
> that fact. Doing nothing and exiting 0 is just wrong.

So, lets say the filter found 123456 sockets matching the filter, and
12345 could be killed

What would be exit status of ss command ?

In this case, there is no black/white answer.

It looks like you have specific needs, you should probably add an option
to ss to have a specific behavior.

But saying current behavior is 'wrong' is subjective.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/18/16 9:47 PM, Eric Dumazet wrote:
> On Wed, 2016-05-18 at 21:02 -0600, David Ahern wrote:
>> On 5/18/16 6:55 PM, Lorenzo Colitti wrote:
>>> On Wed, May 18, 2016 at 3:35 AM, <subashab@codeaurora.org> wrote:
>>>> Would it be acceptable to have a separate column which displays the result of the sock destroy operation per socket.
>>>> State    ... Killed
>>>> ESTAB         Y
>>>> TIME_WAIT     N
>>>
>>> Fine by me, but... what problem are we trying to address? People who
>>> compile their own kernels and don't turn CONFIG_INET_DIAG_DESTROY, and
>>> then are confused why it doesn't work? Seems like we could fix that by
>>> turning CONFIG_INET_DIAG_DESTROY on by default. CCing the people who
>>> commented on the original SOCK_DESTROY patch to see if they have
>>> opinions.
>>
>> The problem is proper feedback to a user. If the kernel supports an
>> action but the action is not enabled the user should get some message to
>> that fact. Doing nothing and exiting 0 is just wrong.
>
> So, lets say the filter found 123456 sockets matching the filter, and
> 12345 could be killed
>
> What would be exit status of ss command ?

Again, I could care less if the exit status is 0 if the user is given "A 
request failed b/c operations is not supported" message. That is feedback.

>
> In this case, there is no black/white answer.
>
> It looks like you have specific needs, you should probably add an option
> to ss to have a specific behavior.
>
> But saying current behavior is 'wrong' is subjective.

You think it is ok to send a request to the kernel, the kernel says "I 
can't do it" and the command says nothing to the user? That is current 
behavior. How on Earth is that acceptable?

I believe my last proposal is that that user gets a single "I could not 
do what you asked" message and nice little summary:

N sockets closed
M sockets failed

If M = total number of sockets then perhaps there is a bigger problem -- 
like a config option is not enabled.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Eric Dumazet]

On Wed, 2016-05-18 at 22:05 -0600, David Ahern wrote:

> You think it is ok to send a request to the kernel, the kernel says "I 
> can't do it" and the command says nothing to the user? That is current 
> behavior. How on Earth is that acceptable?

I don't know. Tell me what is acceptable on a 'dump many sockets' and
some of them can be killed, but not all of them.

What I do know is that you sent totally buggy patches.

If you want to 'fix' something, please send a patch that we can agree
on, ie not breaking existing scripts.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[David Ahern]

On 5/18/16 10:12 PM, Eric Dumazet wrote:
> On Wed, 2016-05-18 at 22:05 -0600, David Ahern wrote:
>
>> You think it is ok to send a request to the kernel, the kernel says "I
>> can't do it" and the command says nothing to the user? That is current
>> behavior. How on Earth is that acceptable?
>
> I don't know. Tell me what is acceptable on a 'dump many sockets' and
> some of them can be killed, but not all of them.
>
> What I do know is that you sent totally buggy patches.

buggy patches? not silently dropping a failure makes for a buggy patch?

>
> If you want to 'fix' something, please send a patch that we can agree
> on, ie not breaking existing scripts.

got it. Google does not care about users; don't un-suppress failures.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Eric Dumazet]

On Thu, 2016-05-19 at 08:06 -0600, David Ahern wrote:
> On 5/18/16 10:12 PM, Eric Dumazet wrote:
> > On Wed, 2016-05-18 at 22:05 -0600, David Ahern wrote:
> >
> >> You think it is ok to send a request to the kernel, the kernel says "I
> >> can't do it" and the command says nothing to the user? That is current
> >> behavior. How on Earth is that acceptable?
> >
> > I don't know. Tell me what is acceptable on a 'dump many sockets' and
> > some of them can be killed, but not all of them.
> >
> > What I do know is that you sent totally buggy patches.
> 
> buggy patches? not silently dropping a failure makes for a buggy patch?

You sent one kernel patch that was useless, then an iproute2 patch that
was simply aborting the dump.

Really, if you want fix things, do this properly instead of simply
ranting about work done by others, even if they are working for Google.

Thank you.

Re: [PATCH iproute2] ss: Tell user about -EOPNOTSUPP for SOCK_DESTROY

[Lorenzo Colitti]

On Thu, May 19, 2016 at 11:06 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> got it. Google does not care about users; don't un-suppress failures.

The users I was trying to care about are the ones that have correctly
configured kernels. It did not seem useful to those users to litter
the dumps with error messages saying that the kernel could not close a
socket that cannot be closed.

Re: [PATCH] net: diag: Tell user if support for destroying TCP sockets is not enabled

[Lorenzo Colitti]

On Tue, May 17, 2016 at 8:53 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> +#else
> +static int tcp_diag_destroy(struct sk_buff *in_skb,
> +                           const struct inet_diag_req_v2 *req)
> +{
> +       return -EOPNOTSUPP;
> +}
>  #endif

I don't understand why you need this. inet_diag_cmd_exact already
returns EOPNOTSUPP if tcp_diag_handler.destroy is NULL:

        else if (cmd == SOCK_DIAG_BY_FAMILY)
                err = handler->dump_one(in_skb, nlh, req);
        else if (cmd == SOCK_DESTROY && handler->destroy)
                err = handler->destroy(in_skb, req);
        else
                err = -EOPNOTSUPP;

Is this not working for some reason?

Re: [PATCH] net: diag: Tell user if support for destroying TCP sockets is not enabled

[David Ahern]

On 5/16/16 6:49 PM, Lorenzo Colitti wrote:
> On Tue, May 17, 2016 at 8:53 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> +#else
>> +static int tcp_diag_destroy(struct sk_buff *in_skb,
>> +                           const struct inet_diag_req_v2 *req)
>> +{
>> +       return -EOPNOTSUPP;
>> +}
>>  #endif
>
> I don't understand why you need this. inet_diag_cmd_exact already
> returns EOPNOTSUPP if tcp_diag_handler.destroy is NULL:
>
>         else if (cmd == SOCK_DIAG_BY_FAMILY)
>                 err = handler->dump_one(in_skb, nlh, req);
>         else if (cmd == SOCK_DESTROY && handler->destroy)
>                 err = handler->destroy(in_skb, req);
>         else
>                 err = -EOPNOTSUPP;
>
> Is this not working for some reason?
>

hmmm.... kernel patch is not needed. Suppression was happening in ss.