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

[Mohsin Bashir <mohsin.bashr@gmail.com>]

On 5/14/26 8:15 AM, David Ahern wrote:
> 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?
> 

It is just an arbitrary value and there's no uapi basis for this. I can 
switch to dynamic allocation/re-allocation.

>> +
>> +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.

Yeah, that's a good suggestion.

> 
>> +			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.
> 
> 

Sure.