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

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri,  8 May 2026 19:23:47 -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) needed for TX scheduling, and introduces the group command
> which ties leaf shapers to a parent node in a single operation.
> 
> Changelog:
> 
>   Patch 1:
>   - parse_scope(): use loop over net_shaper_scope_names[]
>     instead of hardcoded string comparisons
>   - parse_rate(): replace magic number 8 with BITS_PER_BYTE
> 
>   Patch 2:
>   - Add has_bw_min/has_bw_max/has_weight booleans so zero-valued
>     attrs are correctly sent
>   - Add has_handle_id; only send handle id when explicitly provided
>   - Keep node id required in do_cmd(); only netdev scope has
>     optional id
>   - Only emit rate/weight attrs for set command, not show/delete
>   - Validate ll_name_to_index() return, error on unknown device
> 
>   Patch 3:
>   - Use print_rate() for bandwidth display instead of truncating
>     to integer Mbps
> 
>   Patch 4 (new):
>   - Extract struct shaper_args and parse_shaper_arg() helper to
>     share argument parsing between do_cmd() and do_group()
> 
>   Patch 5 (was v1 patch 5, v1 patch 4 dropped):
>   - Reuse parse_shaper_arg() for common argument parsing
>   - Validate group handle and parent scopes are node or netdev
>   - Require parent id when parent scope is node
>   - Fold optional node handle id into do_group() only
>   - Define NET_SHAPER_MAX_LEAVES (256) for leaves array size
> 
>   Patch 6:
>   - Document node id as required for set/show/delete, optional
>     only for group
>   - Add author to man page
> 
> V1: https://lore.kernel.org/netdev/20260501011611.3533573-1-mohsin.bashr@gmail.com/
> 
> Mohsin Bashir (6):
>   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: Extract struct shaper_args and parse_shaper_arg() helper
>   netshaper: Add group command for creating scheduling hierarchies
>   netshaper: Update man page for new parameters and group command
> 
>  include/utils.h       |   1 +
>  man/man8/netshaper.8  | 140 ++++++++++--
>  netshaper/netshaper.c | 485 ++++++++++++++++++++++++++++++++++--------
>  3 files changed, 522 insertions(+), 104 deletions(-)
> 

Automated AI review is not setup for iproute2-next so ran it manually.

Subject: Re: [PATCH iproute2-next v2 0/6] netshaper: Extend netshaper support

On Fri, 8 May 2026, Mohsin Bashir wrote:

> This series extends the netshaper CLI with missing parameter support
> and adds the group command for building scheduling hierarchies.

Thanks for the patches. One issue found in patch 5:

On Fri, 8 May 2026 19:23:52 -0700, Mohsin Bashir wrote:
> +static int do_group(int argc, char **argv)
> +{
[...]
> +	err = rtnl_talk(&gen_rth, &req.n, &answer);
> +	if (err < 0) {
> +		printf("Kernel command failed: %d\n", err);
> +		return err;
> +	}

This error message uses printf() to write to stdout, which will corrupt
JSON output if enabled. Error messages must always go to stderr to preserve
the integrity of JSON output when using the -json flag.

This should be:
		fprintf(stderr, "Kernel command failed: %d\n", err);

The rest of the series looks good. All other error messages correctly
use fprintf(stderr, ...), the new parse_scope() helper properly uses
strcmp() instead of matches(), and the print_rate() usage for bandwidth
display is a nice improvement over the previous truncated integer Mbps
display.

With that fix:
Reviewed-by: Claude Assistant <noreply@anthropic.com>