Re: [patch iproute2-next v2 3/5] devlink: introduce support for netns id for nested handle

[Jiri Pirko <jiri@resnulli.us>]

Tue, Sep 19, 2023 at 04:03:27PM CEST, dsahern@gmail.com wrote:
>On 9/19/23 5:56 AM, Jiri Pirko wrote:
>> @@ -2723,6 +2725,40 @@ static bool should_arr_last_handle_end(struct dl *dl, const char *bus_name,
>>  	       !cmp_arr_last_handle(dl, bus_name, dev_name);
>>  }
>>  
>> +struct netns_name_by_id_ctx {
>> +	int32_t id;
>> +	char *name;
>> +	struct rtnl_handle *rth;
>> +};
>> +
>> +static int netns_name_by_id_func(char *nsname, void *arg)
>> +{
>> +	struct netns_name_by_id_ctx *ctx = arg;
>> +	int32_t ret;
>> +
>> +	ret = netns_netnsid_from_name(ctx->rth, nsname);
>> +	if (ret < 0 || ret != ctx->id)
>> +		return 0;
>> +	ctx->name = strdup(nsname);
>> +	return 1;
>> +}
>> +
>> +static char *netns_name_by_id(int32_t id)
>> +{
>> +	struct rtnl_handle rth;
>> +	struct netns_name_by_id_ctx ctx = {
>> +		.id = id,
>> +		.rth = &rth,
>> +	};
>> +
>> +	if (rtnl_open(&rth, 0) < 0)
>> +		return NULL;
>> +	netns_foreach(netns_name_by_id_func, &ctx);
>> +	rtnl_close(&rth);
>> +
>> +	return ctx.name;
>> +}
>> +
>
>The above is not devlink specific, so it should go in lib/namespace.c as
>well.
>
>Name wise it should be consistent with the last patch, so either
>netns_id_to_name or netns_name_from_id based on the name from the
>refactoring in patch 2.

Okay.

>
>
>>  static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>  {
>>  	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
>> @@ -2740,6 +2776,30 @@ static void pr_out_nested_handle(struct nlattr *nla_nested_dl)
>>  	sprintf(buf, "%s/%s", mnl_attr_get_str(tb[DEVLINK_ATTR_BUS_NAME]),
>>  		mnl_attr_get_str(tb[DEVLINK_ATTR_DEV_NAME]));
>>  	print_string(PRINT_ANY, "nested_devlink", " nested_devlink %s", buf);
>> +
>> +	if (tb[DEVLINK_ATTR_NETNS_ID]) {
>> +		int32_t id = mnl_attr_get_u32(tb[DEVLINK_ATTR_NETNS_ID]);
>> +
>> +		if (id >= 0) {
>> +			char *name = netns_name_by_id(id);
>> +
>> +			if (name) {
>> +				print_string(PRINT_ANY,
>> +					     "nested_devlink_netns",
>> +					     " nested_devlink_netns %s", name);
>> +				free(name);
>> +			} else {
>> +				print_int(PRINT_ANY,
>> +					  "nested_devlink_netnsid",
>> +					  " nested_devlink_netnsid %d", id);
>> +			}
>> +		} else {
>> +			print_string(PRINT_FP, NULL,
>> +				     " nested_devlink_netnsid %s", "unknown");
>> +			print_int(PRINT_JSON,
>> +				  "nested_devlink_netnsid", NULL, id);
>> +		}
>
>Also, devlink in the name here provides no addititional value (devlink
>is the command name) and why add 'nested'? The attribute is just
>NETNS_ID, so why not just 'netnsid' here.

Well, it is a netnsid of the nested devlink instance, not the object
(e.g. port) itself. Omitting that would be misleading. Any idea how to
do this differently?


>
>
>> +	}
>>  }
>>  
>>  static void __pr_out_handle_start(struct dl *dl, struct nlattr **tb,
>