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

[David Ahern <dsahern@kernel.org>]

On 5/18/26 2:23 PM, Mohsin Bashir 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:
> V4:
>   Patch 2:
>   - Reorder BW_MAX/BW_MIN to BW_MIN/BW_MAX in usage text
> 
>   Patch 5:
>   - Extract parse_scope_id() to share handle/parent scope+id parsing
>   - Extract parse_leaves() with dynamic allocation, removing the
>     static NET_SHAPER_MAX_LEAVES limit
>   - Add per-leaf weight and priority support in group command
>   - Remove inline declarations
>   - Skip parent nest when parent not specified
>   - Use goto free_leaves for error cleanup
> 
>   Patch 6:
>   - Document per-leaf weight and priority usage
> 
> V3: https://lore.kernel.org/netdev/20260511183915.797792-1-mohsin.bashr@gmail.com/
> V2: https://lore.kernel.org/netdev/20260509022353.3470738-1-mohsin.bashr@gmail.com/
> 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  | 150 ++++++++++--
>  netshaper/netshaper.c | 554 +++++++++++++++++++++++++++++++++++-------
>  3 files changed, 597 insertions(+), 108 deletions(-)
> 

Applied the set to iproute2-next.

With those patches applied, I asked Claude (model is Opus 4.7) "check
man/man8/netshaper.8 for errors"; output below. Please send followup
patches or address why current state is ok (ie., why claude is wrong).
Thanks,


● I read the page and compared it against netshaper/netshaper.c. The
page renders without man warnings, so the issues below are content /
accuracy:

  Substantive issues

  1. Missing parent scope restriction. In the PARAMETERS description
(man/man8/netshaper.8:188-198) parent is said to "use the same scope/id
syntax as handle." But the code
  (netshaper/netshaper.c:357-360, parse_scope_id()) rejects anything
other than node or netdev. The man page never says that queue is invalid
as a parent scope. The synopsis at
  man/man8/netshaper.8:52-55 also uses an unrestricted PARENT_SCOPE token.

  2. Missing leaf scope restriction. parse_leaves() at
netshaper/netshaper.c:416-420 rejects any leaf whose scope is not queue.
The PARAMETERS section for leaves
  (man/man8/netshaper.8:200-209) only implies this by example ("Each
leaf is given as scope queue id ID") rather than stating the constraint.
Worth saying it explicitly so the reader
  doesn't think they can drop in a node leaf.

  3. Confusing "scheduling groups" wording on the node scope. Line
95-96: "Shapers representing scheduling groups that can be placed at
arbitrary locations..." — group is also a
  command name, so calling a node "a scheduling group" muddles the
terminology. "Intermediate scheduling nodes" (matching tc-style
language) would be clearer and matches what's said
  two paragraphs later ("scheduling node shaper").

  4. show / delete synopsis shows id as optional for all scopes. Lines
36-42 give [ id HANDLE_ID ] for both commands, but the code requires id
for queue and node scopes on every
  command (netshaper.c:268-282, called from do_cmd() for SET/GET/DELETE
alike). The description does spell this out under the scope list, but
readers who only glance at the synopsis
  will be misled. The set synopsis at line 26-27 has the same problem.

  5. -help synopsis is incomplete. main() strips a leading -
(netshaper.c:608-609), so --help, --Version, --color all work too. The
man page only documents the single-dash forms.
  Minor — but inconsistent with what the binary accepts.


  Style / formatting nits

  6. Bold/italic mixed on literal keywords in the synopsis. dev, handle,
scope, and the scope values are bold; but bw-min, bw-max, weight, id,
parent, leaves get italicized via .RI
  (e.g. lines 26-33, 50-61). Convention in iproute2 man pages is bold
for literal keywords. The page renders fine but the typography is
inconsistent.

  7. { … } [ … ] for leaves is unusual (lines 62-64). Renders as leaves
{ scope queue id ID [ weight WEIGHT ] [ priority PRIO ] } [ ... ]. Curly
braces normally denote a required
  choice; here they're being used as repeat-group brackets. A plain
comment in the description or leaves SPEC [ SPEC ... ] with SPEC defined
separately would be clearer.

  8. .IR DEV etc. Single-argument uses of .IR / .BR (lines 23, 25, 27,
29, 31, 33, 38, 40, 42, …) are equivalent to .I / .B. Cosmetic.

  9. Date in .TH ("7 Oct 2025", line 1) hasn't been bumped despite later
commits (e14b336, 2638fef) updating the man page.

  10. Example 4 wording is slightly ambiguous. "Creates a node shaper
under the netdev parent with a 10 Gbit/s cap" — bw-max 10gbit applies to
the node (handle), but a reader could
  attach it to "netdev parent." Tightening to "…with a 10 Gbit/s cap on
the node" would remove the ambiguity.

  The most worth-fixing items are #1, #2, #3, and #9.