[PATCH] Fix ss Netid column and Local/Peer_Address

When using ss -Hutn4 or -utn3, Netid and State columns are sometime merged, it 
can be confusing when trying to pipe into awk or column.
Details (before and after output) are available on this github issue: https://
github.com/shemminger/iproute2/issues/20

Signed-off-by: YoyPa <redacted>
---
 misc/ss.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index c8970438..5e46cc0e 100644
--- a/misc/ss.c
+++ b/misc/ss.c

@@ -144,9 +144,9 @@ static struct column columns[] = {
        { ALIGN_LEFT,   "State",                " ",    0, 0, 0 },
        { ALIGN_LEFT,   "Recv-Q",               " ",    0, 0, 0 },
        { ALIGN_LEFT,   "Send-Q",               " ",    0, 0, 0 },
-       { ALIGN_RIGHT,  "Local Address:",       " ",    0, 0, 0 },
+       { ALIGN_RIGHT,  "Local_Address:",       " ",    0, 0, 0 },
        { ALIGN_LEFT,   "Port",                 "",     0, 0, 0 },
-       { ALIGN_RIGHT,  "Peer Address:",        " ",    0, 0, 0 },
+       { ALIGN_RIGHT,  "Peer_Address:",        " ",    0, 0, 0 },
        { ALIGN_LEFT,   "Port",                 "",     0, 0, 0 },
        { ALIGN_LEFT,   "",                     "",     0, 0, 0 },
 };

@@ -1334,7 +1334,7 @@ static void sock_state_print(struct sockstat *s)
                out("`- %s", sctp_sstate_name[s->state]);
        } else {
                field_set(COL_NETID);
-               out("%s", sock_name);
+               out("%-6s", sock_name);
                field_set(COL_STATE);
                out("%s", sstate_name[s->state]);
        }

-- 
2.19.1

On Fri, 26 Oct 2018 22:53:32 +0200
"Yoann P." [off-list ref] wrote:

Thank for your patch, it does address a  bug.
But iproute2 uses kernel coding style and your patch uses spaces instead
of tabs.

WARNING: please, no spaces at the start of a line
#35: FILE: misc/ss.c:147:
+       { ALIGN_RIGHT,  "Local_Address:",       " ",    0, 0, 0 },$

WARNING: please, no spaces at the start of a line
#38: FILE: misc/ss.c:149:
+       { ALIGN_RIGHT,  "Peer_Address:",        " ",    0, 0, 0 },$

ERROR: code indent should use tabs where possible
#47: FILE: misc/ss.c:1337:
+               out("%-6s", sock_name);$

WARNING: please, no spaces at the start of a line
#47: FILE: misc/ss.c:1337:
+               out("%-6s", sock_name);$

Hi Yohann,

On Fri, 26 Oct 2018 22:53:32 +0200
"Yoann P." [off-list ref] wrote:

When using ss -Hutn4 or -utn3, Netid and State columns are sometime merged, it 
can be confusing when trying to pipe into awk or column.

Thanks for fixing this. A few comments though:

This is needed only if you pipe the output to column(1), I don't think
it's a bug, because printing the header when you pass the output to
column(1) makes little sense -- one should use -H then.

By the way, why do you use column(1), when ss already prints output in
columns? Any other issue you are working around?

I could reproduce this issue with a 70-columns terminal and the options
you gave.

Anyway, I don't think this is the right way to fix it: this will waste
one to two columns in case we have three letters for the Netid
specifier, and won't work the day we get six-letters names. In general,
it looks like a bad idea to reintroduce hardcoded width counts.

The actual issue seems to be that in some cases the left delimiter for
the State column is not printed, and I think you should fix that
instead. I'll look into this within a couple of days and give you some
more specific hints in case you still need them by then.

-- 
Stefano

On Mon, 29 Oct 2018 19:20:36 +0100
Stefano Brivio [off-list ref] wrote:

The actual issue seems to be that in some cases the left delimiter for
the State column is not printed

Much worse, we always print the left delimiter of the last buffered
column, which is usually empty. My bad.

The issue is not so visible in general as we almost always have spaces
to distribute around, but not if you start going below 70/75 columns.
Can you try this?

diff --git a/misc/ss.c b/misc/ss.c
index f99b6874c228..90986b1dc15f 100644
--- a/misc/ss.c
+++ b/misc/ss.c

@@ -1260,7 +1260,7 @@ static void render(void)
        while (token) {
                /* Print left delimiter only if we already started a line */
                if (line_started++)
-                       printed = printf("%s", current_field->ldelim);
+                       printed = printf("%s", f->ldelim);
                else
                        printed = 0;
 

-- 
Stefano

Hi Yohann,

On Fri, 26 Oct 2018 22:53:32 +0200

"Yoann P." [off-list ref] wrote:

quoted

When using ss -Hutn4 or -utn3, Netid and State columns are sometime
merged, it can be confusing when trying to pipe into awk or column.

Thanks for fixing this. A few comments though:

quoted

@@ -144,9 +144,9 @@ static struct column columns[] = {

        { ALIGN_LEFT,   "State",                " ",    0, 0, 0 },
        { ALIGN_LEFT,   "Recv-Q",               " ",    0, 0, 0 },
        { ALIGN_LEFT,   "Send-Q",               " ",    0, 0, 0 },

-       { ALIGN_RIGHT,  "Local Address:",       " ",    0, 0, 0 },
+       { ALIGN_RIGHT,  "Local_Address:",       " ",    0, 0, 0 },

        { ALIGN_LEFT,   "Port",                 "",     0, 0, 0 },

-       { ALIGN_RIGHT,  "Peer Address:",        " ",    0, 0, 0 },
+       { ALIGN_RIGHT,  "Peer_Address:",        " ",    0, 0, 0 },

This is needed only if you pipe the output to column(1), I don't think
it's a bug, because printing the header when you pass the output to
column(1) makes little sense -- one should use -H then.

I don't really care about this modification, I came across it while making the 
github issue example, seemed to be little change, so I dit it.

By the way, why do you use column(1), when ss already prints output in
columns? Any other issue you are working around?

column can hide columns with "-H -" and is a bit faster than awk to output a 
single column according to time, it's the only reason I mentioned it.

quoted

        { ALIGN_LEFT,   "Port",                 "",     0, 0, 0 },
        { ALIGN_LEFT,   "",                     "",     0, 0, 0 },
 
 };

@@ -1334,7 +1334,7 @@ static void sock_state_print(struct sockstat *s)

                out("`- %s", sctp_sstate_name[s->state]);
        
        } else {
        
                field_set(COL_NETID);

-               out("%s", sock_name);
+               out("%-6s", sock_name);

I could reproduce this issue with a 70-columns terminal and the options
you gave.

Anyway, I don't think this is the right way to fix it: this will waste
one to two columns in case we have three letters for the Netid
specifier, and won't work the day we get six-letters names. In general,
it looks like a bad idea to reintroduce hardcoded width counts.

I agree, I just not found the proper way to do it (Not a programmer).

The actual issue seems to be that in some cases the left delimiter for
the State column is not printed, and I think you should fix that
instead. I'll look into this within a couple of days and give you some
more specific hints in case you still need them by then.

I can't reproduce the issue with this modification. :).

On Mon, 29 Oct 2018 21:06:35 +0100
"Yoann P." [off-list ref] wrote:

quoted

By the way, why do you use column(1), when ss already prints output in
columns? Any other issue you are working around?  

column can hide columns with "-H -" and is a bit faster than awk to output a 
single column according to time, it's the only reason I mentioned it.

Okay, but why do you need to hide some columns in the first place? I'm
wondering if your use case would justify adding options to print
selected columns only, in a generic way (right now, you can only
disable some).

Another possibility would be to rename "Local Address:" to "Local:" and
"Peer Address:" to "Peer:" -- in some cases (UNIX sockets) it's already
not so much of an address, more of a path, and "Address" doesn't really
add value when the field contains an address.

I don't like too much "Local_Address:" and "Peer_Address:" as the
output is supposed to be human-readable by default, and that underscore
just doesn't fit.

-- 
Stefano

On Mon, 29 Oct 2018 21:07:47 +0100
"Yoann P." [off-list ref] wrote:

quoted

-                       printed = printf("%s", current_field->ldelim);
+                       printed = printf("%s", f->ldelim);
                else
                        printed = 0;  

I can't reproduce the issue with this modification. :).

Thanks a lot for testing, and especially for reporting this! I'll
submit this as a patch in a moment.

-- 
Stefano

Le lundi 29 octobre 2018, 23:03:07 CET Stefano Brivio a écrit :

On Mon, 29 Oct 2018 21:06:35 +0100

"Yoann P." [off-list ref] wrote:

quoted

quoted

By the way, why do you use column(1), when ss already prints output in
columns? Any other issue you are working around?

column can hide columns with "-H -" and is a bit faster than awk to output
a single column according to time, it's the only reason I mentioned it.

Okay, but why do you need to hide some columns in the first place? I'm
wondering if your use case would justify adding options to print
selected columns only, in a generic way (right now, you can only
disable some).

Another possibility would be to rename "Local Address:" to "Local:" and
"Peer Address:" to "Peer:" -- in some cases (UNIX sockets) it's already
not so much of an address, more of a path, and "Address" doesn't really
add value when the field contains an address.

I don't like too much "Local_Address:" and "Peer_Address:" as the
output is supposed to be human-readable by default, and that underscore
just doesn't fit.

I send the peer address column to geoiplookup (currently changing to 
mmdblookup as geoip database is replaced by geolite2) to recover Country, 
Asnum and ASname of peers.