| Message ID | 1463442791-2399-2-git-send-email-dsa@cumulusnetworks.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
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).
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.
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.
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 {
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(-)