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?

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.