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

[Jiri Pirko <jiri@resnulli.us>]

Wed, Sep 20, 2023 at 09:30:01AM CEST, jiri@resnulli.us wrote:
>Tue, Sep 19, 2023 at 08:48:29PM CEST, dsahern@gmail.com wrote:
>>On 9/19/23 11:19 AM, Jiri Pirko wrote:
>>>>
>>>>>  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?
>>> 
>>> 
>>
>>The attribute is a namespace id, and the value is a namespace id. Given
>>that, the name here should be netnsid (or nsid - we did a horrible job
>>with consistency across iproute2 commands). I have not followed the
>>kernel patches to understand what you mean by nested devlink instance.
>
>Please do that. Again, the netnsid is related to the nested instance.
>Therefore I put the "nested_devlink" in the name. Putting just "netnsid"
>as you suggest is wrong. Another possibility would be do nest this into
>object, but:
>1) I didn't find nice way to do that
>2) We would break linecards as they expose nested_devlink already

Did you have a chance to check this? I have v3 ready for submission with
the other changes you requested.

Thanks!


>
>IDK :/