[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZXdN14BQwrUcpbgt@debian>
Date: Mon, 11 Dec 2023 18:58:47 +0100
From: Guillaume Nault <gnault@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: dsahern@...il.com, edumazet@...gle.com, mkubecek@...e.cz,
netdev@...r.kernel.org
Subject: Re: [PATCH iproute2-next] ss: Add support for dumping TCP
bound-inactive sockets.
On Mon, Dec 11, 2023 at 11:19:17PM +0900, Kuniyuki Iwashima wrote:
> > diff --git a/misc/ss.c b/misc/ss.c
> > index 16ffb6c8..19adc1b7 100644
> > --- a/misc/ss.c
> > +++ b/misc/ss.c
> > @@ -210,6 +210,8 @@ enum {
> > SS_LAST_ACK,
> > SS_LISTEN,
> > SS_CLOSING,
> > + SS_NEW_SYN_RECV,
>
> I think this is bit confusing as TCP_NEW_SYN_RECV is usually
> invisible from user.
>
> TCP_NEW_SYN_RECV was originally split from TCP_SYN_RECV for a
> non-{TFO,cross-SYN} request.
>
> So, both get_openreq4() (/proc/net/tcp) and inet_req_diag_fill()
> (inet_diag) maps TCP_NEW_SYN_RECV to TCP_SYN_RECV.
I think we need to explicitely set SS_NEW_SYN_RECV anyway, because we
have to set its entry in the sstate_namel array in scan_state().
But I can add a comment like this:
@@ -210,6 +210,8 @@ enum {
SS_LAST_ACK,
SS_LISTEN,
SS_CLOSING,
+ SS_NEW_SYN_RECV, /* Kernel only value, not for use in user space */
+ SS_BOUND_INACTIVE,
SS_MAX
};
> > + SS_BOUND_INACTIVE,
>
> I prefer explicitly assigning a number to SS_BOUND_INACTIVE.
>
>
> > SS_MAX
> > };
> >
> > @@ -1382,6 +1384,8 @@ static void sock_state_print(struct sockstat *s)
> > [SS_LAST_ACK] = "LAST-ACK",
> > [SS_LISTEN] = "LISTEN",
> > [SS_CLOSING] = "CLOSING",
> > + [SS_NEW_SYN_RECV] = "NEW-SYN-RECV",
>
> If we need to define SS_NEW_SYN_RECV, I prefer not setting
> it or setting "" or "SYN-RECV".
I originally considered the string wasn't important as the kernel isn't
supposed to return this value. But I agree we can do better.
I don't really like "SYN-RECV" though, as I find it even more confusing.
I'd prefer your other proposal using "", or maybe "UNDEF", together with
a comment saying something like "Never returned by kernel".
> > + [SS_BOUND_INACTIVE] = "BOUND-INACTIVE",
And whatever we decide for SS_NEW_SYN_RECV, we can apply the same to
SS_BOUND_INACTIVE, since the kernel isn't supposed set this state on
output too.
So we'd have something like:
@@ -1382,6 +1384,8 @@ static void sock_state_print(struct sockstat *s)
[SS_LAST_ACK] = "LAST-ACK",
[SS_LISTEN] = "LISTEN",
[SS_CLOSING] = "CLOSING",
+ [SS_NEW_SYN_RECV] = "UNDEF", /* Never returned by kernel */
+ [SS_BOUND_INACTIVE] = "UNDEF", /* Never returned by kernel */
};
> > @@ -5421,6 +5426,8 @@ static int scan_state(const char *state)
> > [SS_LAST_ACK] = "last-ack",
> > [SS_LISTEN] = "listening",
> > [SS_CLOSING] = "closing",
> > + [SS_NEW_SYN_RECV] = "new-syn-recv",
>
> Same here.
Well, here, whatever we do, the string associated with SS_NEW_SYN_RECV
will be usable by the user. So I'd prefer to have a specific and
unambiguous string and then explicitly refuse it.
What do you think of something like the following?
@@ -5421,9 +5426,17 @@ static int scan_state(const char *state)
[SS_LAST_ACK] = "last-ack",
[SS_LISTEN] = "listening",
[SS_CLOSING] = "closing",
+ [SS_NEW_SYN_RECV] = "new-syn-recv",
+ [SS_BOUND_INACTIVE] = "bound-inactive",
};
int i;
+ /* NEW_SYN_RECV is a kernel implementation detail. It shouldn't be used
+ * or even be visible to the user.
+ */
+ if (strcasecmp(state, "new-syn-recv") == 0)
+ goto wrong_state;
+
if (strcasecmp(state, "close") == 0 ||
strcasecmp(state, "closed") == 0)
return (1<<SS_CLOSE);
@@ -5446,6 +5459,7 @@ static int scan_state(const char *state)
return (1<<i);
}
+wrong_state:
fprintf(stderr, "ss: wrong state name: %s\n", state);
exit(-1);
}
Powered by blists - more mailing lists