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

[David Ahern <dsahern@gmail.com>]

On 10/22/23 1:41 AM, Patrisious Haddad wrote:
> 
> 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?
> 
> 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.

tools packaged with iproute2 should use common code where possible.