Messages in this thread | | | Date | Sun, 22 Oct 2023 12:22:03 +0300 | Subject | Re: [PATCH iproute2-next 2/3] rdma: Add an option to set privileged QKEY parameter | From | Patrisious Haddad <> |
| |
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.
| |