[PATCH] Fix ss Netid column and Local/Peer_Address

[PATCH] Fix ss Netid column and Local/Peer_Address

[Yoann P.]

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 <yoann.p.public@gmail.com>
---
 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

Re: [PATCH] Fix ss Netid column and Local/Peer_Address

[Stephen Hemminger]

On Fri, 26 Oct 2018 22:53:32 +0200
"Yoann P." <yoann.p.public@gmail.com> 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.
> Details (before and after output) are available on this github issue: https://
> github.com/shemminger/iproute2/issues/20
> 
> Signed-off-by: YoyPa <yoann.p.public@gmail.com>
> ---
>  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]);
>         }

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);$

Re: [PATCH] Fix ss Netid column and Local/Peer_Address

[Stefano Brivio]

Hi Yohann,

On Fri, 26 Oct 2018 22:53:32 +0200
"Yoann P." <yoann.p.public@gmail.com> 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:

> @@ -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.

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

>         { 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.

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

Re: [PATCH] Fix ss Netid column and Local/Peer_Address

[Stefano Brivio]

On Mon, 29 Oct 2018 19:20:36 +0100
Stefano Brivio <sbrivio@redhat.com> 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

Re: [PATCH] Fix ss Netid column and Local/Peer_Address

[Yoann P.]

> Hi Yohann,
> 
> On Fri, 26 Oct 2018 22:53:32 +0200
> 
> "Yoann P." <yoann.p.public@gmail.com> 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:
> > @@ -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.
> 
> >         { 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.

Re: [PATCH] Fix ss Netid column and Local/Peer_Address

[Yoann P.]

> On Mon, 29 Oct 2018 19:20:36 +0100
> 
> Stefano Brivio <sbrivio@redhat.com> 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;


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

Re: [PATCH] Fix ss Netid column and Local/Peer_Address

[Stefano Brivio]

On Mon, 29 Oct 2018 21:06:35 +0100
"Yoann P." <yoann.p.public@gmail.com> wrote:

> > 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

Re: [PATCH] Fix ss Netid column and Local/Peer_Address

[Stefano Brivio]

On Mon, 29 Oct 2018 21:07:47 +0100
"Yoann P." <yoann.p.public@gmail.com> wrote:

> > -                       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

Re: [PATCH] Fix ss Netid column and Local/Peer_Address

[Yoann P.]

Le lundi 29 octobre 2018, 23:03:07 CET Stefano Brivio a écrit :
> On Mon, 29 Oct 2018 21:06:35 +0100
> 
> "Yoann P." <yoann.p.public@gmail.com> wrote:
> > > 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.