Re: [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter

[Patrisious Haddad <phaddad@nvidia.com>]

On 10/19/2023 1:38 PM, Petr Machata wrote:
> Patrisious Haddad <phaddad@nvidia.com> writes:
>
>> @@ -40,6 +45,22 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data)
>>   				   mode_str);
>>   	}
>>   
>> +	if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) {
>> +		const char *pqkey_str;
>> +		uint8_t pqkey_mode;
>> +
>> +		pqkey_mode =
>> +			mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]);
>> +
>> +		if (pqkey_mode < ARRAY_SIZE(privileged_qkey_str))
>> +			pqkey_str = privileged_qkey_str[pqkey_mode];
>> +		else
>> +			pqkey_str = "unknown";
>> +
>> +		print_color_string(PRINT_ANY, COLOR_NONE, "privileged-qkey",
>> +				   "privileged-qkey %s ", pqkey_str);
>> +	}
>> +
> Elsewhere in the file, you just use print_color_on_off(), why not here?

About this as I previously answered I don't really see a big difference 
between it and "print_color_string" but if the

maintainer thinks this is an essential change I can fix and re-send.

Thanks for the review.

>
>>   	if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK])
>>   		cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]);
>>   
>> @@ -111,10 +155,25 @@ static int sys_set_netns_args(struct rd *rd)
>>   	return sys_set_netns_cmd(rd, cmd);
>>   }
>>   
>> +static int sys_set_privileged_qkey_args(struct rd *rd)
>> +{
>> +	bool cmd;
>> +
>> +	if (rd_no_arg(rd) || !sys_valid_privileged_qkey_cmd(rd_argv(rd))) {
>> +		pr_err("valid options are: { on | off }\n");
>> +		return -EINVAL;
>> +	}
> This could use parse_on_off().
More importantly I looked a bit more into it, and I prefer not to use 
it, it would also lead to additional error prints that are not 
consistent with what we previously had for this API, so I prefer to keep 
it as is , so that the error messages for all arguments of this command 
be identical.
>
>> +
>> +	cmd = (strcmp(rd_argv(rd), "on") == 0) ? true : false;
>> +
>> +	return sys_set_privileged_qkey_cmd(rd, cmd);
>> +}
>> +
>>   static int sys_set_help(struct rd *rd)
>>   {
>>   	pr_out("Usage: %s system set [PARAM] value\n", rd->filename);
>>   	pr_out("            system set netns { shared | exclusive }\n");
>> +	pr_out("            system set privileged-qkey { on | off }\n");
>>   	return 0;
>>   }
>>   
>> @@ -124,6 +183,7 @@ static int sys_set(struct rd *rd)
>>   		{ NULL,			sys_set_help },
>>   		{ "help",		sys_set_help },
>>   		{ "netns",		sys_set_netns_args},
>> +		{ "privileged-qkey",	sys_set_privileged_qkey_args},
>>   		{ 0 }
>>   	};
> The rest of the code looks sane to me, but I'm not familiar with the
> feature.