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

[Patrisious Haddad]

On 10/19/2023 1:38 PM, Petr Machata wrote:
> Patrisious Haddad <phaddad@xxxxxxxxxx> 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?

The print_color_on_off was used for copy-on-fork which as you see has no 
set function,

I was simply trying to be consistent with this file convention & style, 
whereas print_color_string was used for the other configurable value 
("netns"), I can obviously change that if you all see it as necessary.

> >   	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().
You are absolutely correct, but just as well was trying to maintain same 
code style as the previous configurable value we have here, but I think 
using parse_on_off here can save us some code.
> > +
> > +	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.
If no one else has any comments soon, and these two comments are 
actually considered critical I can re-send my patches with those issues 
fixed.