| Message ID | 5185850.l7bAsbzJZX@yo-gs |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | Fix ss Netid column and Local/Peer_Address | expand |
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);$
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.
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;
> 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.
> 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. :).
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.
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.
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.
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]); }
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(-)