Message-ID: <20250829130915.52a1cc5a@elisabeth>
Date: Fri, 29 Aug 2025 13:09:15 +0200
From: Stefano Brivio <sbrivio@...hat.com>
To: Paul Wayper <pwayper@...hat.com>
Cc: Stephen Hemminger <stephen@...workplumber.org>, netdev@...r.kernel.org,
 paulway@...hat.com, jbainbri@...hat.com
Subject: Re: [PATCH iproute2] ss: Don't pad the last (enabled) column

Here comes a detailed, but partial review.

It's partial because, as I mentioned on the v1 thread, I don't quite
grasp the issue you're facing yet, and I couldn't reproduce it.

On Tue, 26 Aug 2025 10:22:37 +1000
Paul Wayper <pwayper@...hat.com> wrote:

> ss will emit spaces on the right hand side of a left-justified, enabled
> column even if it's the last column.  In situations where one or more
> lines are very long - e.g. because of a large PROCESS field value - this
> causes a lot of excess output.
> 
> Firstly, calculate the last enabled column.  Then use this in the check
> for whether to emit trailing spaces on the last column.
> 
> Also name the 'EXT' column as 'Details' and mark it as disabled by
> default, enabled when the -e or --extended options are supplied.
> 
> Fixes: 59f46b7b5be86 ("ss: Introduce columns lightweight abstraction")
> Signed-off-by: Paul Wayper <paulway@...hat.com>
> ---
>  misc/ss.c | 42 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index 989e168ae35026249ccec0e2d4a3df07b0438c7b..1c576c1e5997ccbca448b6ed5af3d41d7867ba76 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -127,6 +127,8 @@ enum col_id {
>  	COL_MAX
>  };
>  
> +int LAST_COL = COL_MAX;

Uppercase identifiers are typically used for build-time constants
(#define directives). It would be good to have this one together with
all the other global variables (just after 'int oneline').

> +
>  enum col_align {
>  	ALIGN_LEFT,
>  	ALIGN_CENTER,
> @@ -151,8 +153,8 @@ static struct column columns[] = {
>  	{ ALIGN_LEFT,	"Port",			"",	0, 0, 0 },
>  	{ ALIGN_RIGHT,	"Peer Address:",	" ",	0, 0, 0 },
>  	{ ALIGN_LEFT,	"Port",			"",	0, 0, 0 },
> -	{ ALIGN_LEFT,	"Process",		"",	0, 0, 0 },
> -	{ ALIGN_LEFT,	"",			"",	0, 0, 0 },
> +	{ ALIGN_LEFT,	"Process",		" ",	0, 0, 0 },

This should be a separate patch with its own tag:

Fixes: 5883c6eba517 ("ss: show header for --processes/-p")

> +	{ ALIGN_LEFT,	"Details",		" ",	1, 0, 0 },

What does "Details" add? It's very different types of details, and
that's the reason why this column has no header.

>  };
>  
>  static struct column *current_field = columns;
> @@ -1079,6 +1081,22 @@ static void out(const char *fmt, ...)
>  	va_end(args);
>  }
>  
> +static void check_last_column(void)

That's not what this function does. It looks for the last column and
sets a variable, so it could be called find_last_column() or
set_last_column_index(), for instance.

> +{
> +	/* Find the last non-disabled column and set LAST_COL. */

...and once the name is clear, this comment can go away.

> +	for (int i = COL_MAX - 1; i > 0; i--) {

While it's allowed in C99, in this file, variables are never declared in
loop headers.

You don't actually need 'i', you could directly use the variable of the
index you're calculating.

It's not entirely obvious why you would start from the right / last
column, it looks more intuitive to me to start from the left.

> +		if (!columns[i].disabled) {
> +			LAST_COL = i;
> +			return;

This assumes that, if a column is disabled, the previous one is always
the last column, which isn't correct. You could have COL_PROC disabled,
but COL_EXT enabled.

> +		}
> +	}
> +}
> +
> +static int field_is_last(struct column *f)
> +{
> +	return f - columns == LAST_COL;
> +}
> +
>  static int print_left_spacing(struct column *f, int stored, int printed)
>  {
>  	int s;
> @@ -1104,6 +1122,10 @@ static void print_right_spacing(struct column *f, int printed)
>  	if (!f->width || f->align == ALIGN_RIGHT)
>  		return;
>  
> +	/* Don't print trailing space if this is the last column. */
> +	if (field_is_last(f))
> +		return;

The caller, render(), already deals with this kind of considerations,
such as printing newlines if field_is_last(). It would be more natural
to *not* call print_right_spacing() right away.

> +
>  	s = f->width - printed;
>  	if (f->align == ALIGN_CENTER)
>  		s /= 2;
> @@ -1143,11 +1165,6 @@ static void field_flush(struct column *f)
>  	buffer.tail->end = buffer.cur->data;
>  }
>  
> -static int field_is_last(struct column *f)
> -{
> -	return f - columns == COL_MAX - 1;
> -}
> -
>  /* Get the next available token in the buffer starting from the current token */
>  static struct buf_token *buf_token_next(struct buf_token *cur)
>  {
> @@ -1316,6 +1333,9 @@ static void render(void)
>  	if (!buffer.head)
>  		return;
>  
> +	/* Find last non-disabled column */
> +	check_last_column();

You're calling this in render(), but other functions rely on
field_is_last(), and they might be called before render(), or without
render() being called at all.

Ideally, this should be set once the command line is processed, in
main(). See 'oneline', 'show_header', etc. Alternatively,
field_is_last() could just calculate the index of the last column
itself, it shouldn't be significantly more expensive than a subtraction.

That is, unless you want to have it for render() only, I'm not quite
sure about the intention at this point. But then there's no need for a
global variable.

> +
>  	token = (struct buf_token *)buffer.head->data;
>  
>  	/* Ensure end alignment of last token, it wasn't necessarily flushed */
> @@ -2452,12 +2472,12 @@ static void proc_ctx_print(struct sockstat *s)
>  		if (find_entry(s->ino, &buf,
>  				(show_proc_ctx & show_sock_ctx) ?
>  				PROC_SOCK_CTX : PROC_CTX) > 0) {
> -			out(" users:(%s)", buf);
> +			out("users:(%s)", buf);

Unrelated change.

>  			free(buf);
>  		}
>  	} else if (show_processes || show_threads) {
>  		if (find_entry(s->ino, &buf, USERS) > 0) {
> -			out(" users:(%s)", buf);
> +			out("users:(%s)", buf);

Unrelated change.

>  			free(buf);
>  		}
>  	}
> @@ -6241,6 +6261,10 @@ int main(int argc, char *argv[])
>  	if (ssfilter_parse(&current_filter.f, argc, argv, filter_fp))
>  		usage();
>  
> +	/* Details column normally disabled, enable if asked */

That should be obvious from variable names (it is for me), if it's not
names should be changed instead of adding a comment.

> +	if (show_details)
> +		columns[COL_EXT].disabled = 0;
> +
>  	if (!show_processes)
>  		columns[COL_PROC].disabled = 1;
>  

-- 
Stefano

Powered by blists - more mailing lists