Re: [PATCH iproute2-next v3 5/6] netshaper: Add group command for creating scheduling hierarchies

[David Ahern <dsahern@kernel.org>]

On 5/11/26 12:39 PM, Mohsin Bashir wrote:
> @@ -319,6 +324,217 @@ static int do_cmd(int argc, char **argv, int cmd)
>  	return err;
>  }
>  
> +#define NET_SHAPER_MAX_LEAVES	256

why 256 as the limit? What drives a max value -- anything related to the
uapi? Will 256 ever be deemed too little? if not, why?

> +
> +static int do_group(int argc, char **argv)
> +{
> +	GENL_REQUEST(req, 4096, genl_family, 0, NET_SHAPER_FAMILY_VERSION,
> +		     NET_SHAPER_CMD_GROUP, NLM_F_REQUEST | NLM_F_ACK);
> +
> +	struct shaper_args args = SHAPER_ARGS_INIT;
> +	bool parsing_leaves = false, has_handle_id = false, has_parent_id = false;
> +	int parent_scope = -1, num_leaves = 0;
> +	int err, ret, handle_scope = NET_SHAPER_SCOPE_UNSPEC;
> +	__u32 handle_id = 0, parent_id = 0;
> +	struct nlmsghdr *answer;
> +
> +	struct {
> +		int scope;
> +		__u32 id;
> +	} leaves[NET_SHAPER_MAX_LEAVES];
> +
> +	while (argc > 0) {
> +		if (parsing_leaves) {


This while loop is really long. Move this parsing_leaves code to a
function and then drop the parsing_leaves flag.

> +			if (strcmp(*argv, "scope") == 0) {
> +				int lscope;
> +
> +				NEXT_ARG();
> +				lscope = parse_scope(*argv);
> +				if (lscope < 0) {
> +					fprintf(stderr, "Invalid leaf scope \"%s\"\n",
> +						*argv);
> +					return -1;
> +				}
> +				NEXT_ARG();
> +				if (strcmp(*argv, "id") != 0) {
> +					fprintf(stderr, "Expected \"id\" after leaf scope\n");
> +					return -1;
> +				}
> +
> +				NEXT_ARG();
> +				if (num_leaves >= ARRAY_SIZE(leaves)) {
> +					fprintf(stderr, "Too many leaves\n");
> +					return -1;
> +				}
> +				leaves[num_leaves].scope = lscope;
> +				if (get_unsigned(&leaves[num_leaves].id, *argv, 10)) {
> +					fprintf(stderr, "Invalid leaf id\n");
> +					return -1;
> +				}
> +				num_leaves++;
> +				argc--;
> +				argv++;
> +				continue;
> +			}
> +			parsing_leaves = false;
> +		}
> +
> +		ret = parse_shaper_arg(*argv, &argc, &argv, &args);
> +		if (ret < 0)
> +			return -1;
> +		if (ret > 0) {
> +			argc--;
> +			argv++;
> +			continue;
> +		}
> +
> +		if (strcmp(*argv, "handle") == 0) {
> +			NEXT_ARG();
> +			if (strcmp(*argv, "scope") != 0) {
> +				fprintf(stderr, "Expected \"scope\" after \"handle\"\n");
> +				return -1;
> +			}
> +			NEXT_ARG();
> +			handle_scope = parse_scope(*argv);
> +			if (handle_scope < 0) {
> +				fprintf(stderr, "Invalid handle scope \"%s\"\n",
> +					*argv);
> +				return -1;
> +			}
> +			if (handle_scope != NET_SHAPER_SCOPE_NODE &&
> +			    handle_scope != NET_SHAPER_SCOPE_NETDEV) {
> +				fprintf(stderr, "Group handle scope must be \"node\" or \"netdev\"\n");
> +				return -1;
> +			}
> +
> +			if (argc > 1 && strcmp(argv[1], "id") == 0) {
> +				NEXT_ARG();
> +				NEXT_ARG();
> +				if (get_unsigned(&handle_id, *argv, 10)) {
> +					fprintf(stderr, "Invalid handle id\n");
> +					return -1;
> +				}
> +				has_handle_id = true;
> +			}
> +		} else if (strcmp(*argv, "parent") == 0) {
> +			NEXT_ARG();
> +			if (strcmp(*argv, "scope") != 0) {
> +				fprintf(stderr, "Expected \"scope\" after \"parent\"\n");
> +				return -1;
> +			}
> +			NEXT_ARG();
> +			parent_scope = parse_scope(*argv);
> +			if (parent_scope < 0) {
> +				fprintf(stderr, "Invalid parent scope \"%s\"\n",
> +					*argv);
> +				return -1;
> +			}
> +			if (parent_scope != NET_SHAPER_SCOPE_NODE &&
> +			    parent_scope != NET_SHAPER_SCOPE_NETDEV) {
> +				fprintf(stderr, "Parent scope must be \"node\" or \"netdev\"\n");
> +				return -1;
> +			}
> +
> +			if (parent_scope == NET_SHAPER_SCOPE_NODE) {
> +				NEXT_ARG();
> +				if (strcmp(*argv, "id") != 0) {
> +					fprintf(stderr, "What is \"%s\"\n", *argv);
> +					usage();
> +					return -1;
> +				}
> +				NEXT_ARG();
> +				if (get_unsigned(&parent_id, *argv, 10)) {
> +					fprintf(stderr, "Invalid parent id\n");
> +					return -1;
> +				}
> +				has_parent_id = true;
> +			} else if (argc > 1 && strcmp(argv[1], "id") == 0) {
> +				NEXT_ARG();
> +				NEXT_ARG();
> +				if (get_unsigned(&parent_id, *argv, 10)) {
> +					fprintf(stderr, "Invalid parent id\n");
> +					return -1;
> +				}
> +				has_parent_id = true;
> +			}
> +		} else if (strcmp(*argv, "leaves") == 0) {
> +			parsing_leaves = true;
> +			argc--;
> +			argv++;
> +			continue;
> +		} else {
> +			fprintf(stderr, "What is \"%s\"\n", *argv);
> +			usage();
> +			return -1;
> +		}
> +		argc--;
> +		argv++;
> +	}
> +
> +	if (args.ifindex == -1)
> +		missarg("dev");
> +	if (handle_scope == NET_SHAPER_SCOPE_UNSPEC)
> +		missarg("handle");
> +	if (parent_scope < 0)
> +		missarg("parent");
> +	if (num_leaves == 0)
> +		missarg("leaves");
> +
> +	addattr32(&req.n, sizeof(req), NET_SHAPER_A_IFINDEX, args.ifindex);
> +
> +	struct rtattr *parent = addattr_nest(&req.n, sizeof(req),
> +					     NET_SHAPER_A_PARENT | NLA_F_NESTED);

no inline declarations.

Claude and Codex like to do this. If this code was co-authored by an AI
tool, please add the Assisted by tag (see
Documentation/process/coding-assistants.rst in the kernel repo). And if
that is true (code is co-authored by AI), drop the Reviewed-by tag.