Re: [PATCH iproute2-next 0/5] netshaper: Extend netshaper support

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 30 Apr 2026 18:16:06 -0700
Mohsin Bashir <mohsin.bashr@gmail.com> wrote:

> From: Mohsin Bashir <hmohsin@meta.com>
> 
> This series extends the netshaper CLI with missing parameter support
> and adds the group command for building scheduling hierarchies.
> 
> The existing netshaper tool only supports setting bw-max on individual
> shapers. This series adds the remaining shaper attributes (bw-min,
> weight, priority) needed for TX scheduling, and introduces the
> group command which ties leaf shapers to a parent node in a single
> operation.
> 
> Mohsin Bashir (5):
>   netshaper: Extract parse_scope() and parse_rate() helpers
>   netshaper: Add bw-min and weight parameter support
>   netshaper: Extend show output with parent, bw-min and weight
>   netshaper: Make handle id optional for node scope
>   netshaper: Add group command for creating scheduling hierarchies
> 
>  netshaper/netshaper.c | 398 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 324 insertions(+), 74 deletions(-)
> 

AI review liked the patch but found a JSON break
As always there are some noise things here.
Like the suggestion about noreturn attribute.

Subject: Re: [PATCH iproute2] netshaper: Add group command and parameter support

On Thu, 30 Apr 2026, Mohsin Bashir wrote:
> This series adds netshaper support for scheduling hierarchies, bw-min/weight
> parameters, and improved output formatting.

Overall the patches look good and follow most iproute2 conventions properly.
I especially appreciate that all new code correctly uses strcmp() instead of
matches() for argument parsing.

However, there are a few minor issues that should be addressed:

> @@ -47,55 +54,98 @@ static const char *net_shaper_scope_names[NET_SHAPER_SCOPE_MAX + 1] = {
>  static void print_netshaper_attrs(struct nlmsghdr *answer)
> [...]
> +	printf("\n");
>  }

The raw printf("\n") at the end of print_netshaper_attrs() breaks JSON output.
All output should use the print_XXX() helpers to maintain proper JSON formatting.
This should be:

	print_nl();

or handled by the print functions themselves. The print helpers ensure that
JSON output remains valid when the -j flag is used.

> @@ -103,13 +153,14 @@ static int do_cmd(int argc, char **argv, int cmd)
> [...]
>  	while (argc > 0) {
>  		if (strcmp(*argv, "dev") == 0) {
>  			NEXT_ARG();
>  			ifindex = ll_name_to_index(*argv);

In do_cmd(), the return value of ll_name_to_index() is not properly validated.
When the device doesn't exist, this function returns 0. The code should check:

		ifindex = ll_name_to_index(*argv);
		if (ifindex == 0) {
			fprintf(stderr, "Device \"%s\" not found\n", *argv);
			return -1;
		}

This validation is correctly done in do_group() but missing in do_cmd().

> @@ -31,13 +31,20 @@ static void usage(void)
>  {
>  	fprintf(stderr,
>  		"Usage: netshaper [ OPTIONS ] { COMMAND | help }\n"

The usage() function should be marked with __attribute__((noreturn)) as per
iproute2 coding conventions:

static void usage(void) __attribute__((noreturn));

static void usage(void)
{
	...
	exit(-1);
}

This helps the compiler with optimization and static analysis.

These are all minor issues in an otherwise well-structured patch series.
The code properly uses designated initializers, validates input with
appropriate helpers, and follows the error handling patterns correctly.

With these small fixes, the series would be ready for inclusion.

Best regards