[PATCH v2 iproute2-next 0/3] Add support to set privileged qkey parameter

[PATCH v2 iproute2-next 0/3] Add support to set privileged qkey parameter

[Patrisious Haddad]

This patchset adds support to enable or disable privileged QKEY.
When enabled, non-privileged users will be allowed to specify a controlled QKEY.
The corresponding kernel commit is yet to be merged so currently there
is no hash but the commit name is
("RDMA/core: Add support to set privileged qkey parameter")

All the information regarding the added parameter and its usage are included
in the commits below and the edited man page.

---
v1->v2:
- Uses print_color_on_off instead of print_color_string for printing.
- Uses parse_on_off instead of manual parsing.

Patrisious Haddad (3):
  rdma: update uapi headers
  rdma: Add an option to set privileged QKEY parameter
  rdma: Adjust man page for rdma system set privileged_qkey command

 man/man8/rdma-system.8                | 32 ++++++++++++++++---
 rdma/include/uapi/rdma/rdma_netlink.h |  6 ++++
 rdma/sys.c                            | 46 +++++++++++++++++++++++++--
 rdma/utils.c                          |  1 +
 4 files changed, 78 insertions(+), 7 deletions(-)

-- 
2.18.1

[PATCH v2 iproute2-next 1/3] rdma: update uapi headers

[Patrisious Haddad]

Update rdma_netlink.h file upto kernel commit 36ce80759f8c
("RDMA/core: Add support to set privileged qkey parameter")

Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
---
 rdma/include/uapi/rdma/rdma_netlink.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
index 92c528a0..3a506efa 100644
--- a/rdma/include/uapi/rdma/rdma_netlink.h
+++ b/rdma/include/uapi/rdma/rdma_netlink.h
@@ -554,6 +554,12 @@ enum rdma_nldev_attr {
 	RDMA_NLDEV_ATTR_STAT_HWCOUNTER_INDEX,	/* u32 */
 	RDMA_NLDEV_ATTR_STAT_HWCOUNTER_DYNAMIC, /* u8 */
 
+	/*
+	 * To enable or disable using privileged_qkey without being
+	 * a privileged user.
+	 */
+	RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE,	/* u8 */
+
 	/*
 	 * Always the end
 	 */
-- 
2.18.1

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

[Patrisious Haddad]

Enrich rdmatool with an option to enable or disable privileged QKEY.
When enabled, non-privileged users will be allowed to specify a
controlled QKEY.

By default this parameter is disabled in order to comply with IB spec.
According to the IB specification rel-1.6, section 3.5.3:
"QKEYs with the most significant bit set are considered controlled
QKEYs, and a HCA does not allow a consumer to arbitrarily specify a
controlled QKEY."

This allows old applications which existed before the kernel commit:
0cadb4db79e1 ("RDMA/uverbs: Restrict usage of privileged QKEYs")
they can use privileged QKEYs without being a privileged user to now
be able to work again without being privileged granted they turn on this
parameter.

rdma tool command examples and output.

$ rdma system show
netns shared privileged-qkey off copy-on-fork on

$ rdma system set privileged-qkey on

$ rdma system show
netns shared privileged-qkey on copy-on-fork on

Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
---
 rdma/sys.c   | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 rdma/utils.c |  1 +
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/rdma/sys.c b/rdma/sys.c
index fd785b25..db34cb41 100644
--- a/rdma/sys.c
+++ b/rdma/sys.c
@@ -40,6 +40,17 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data)
 				   mode_str);
 	}
 
+	if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) {
+		uint8_t pqkey_mode;
+
+		pqkey_mode =
+			mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]);
+
+		print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey",
+				   "privileged-qkey %s ", pqkey_mode);
+
+	}
+
 	if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK])
 		cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]);
 
@@ -67,8 +78,9 @@ static int sys_show_no_args(struct rd *rd)
 static int sys_show(struct rd *rd)
 {
 	const struct rd_cmd cmds[] = {
-		{ NULL,		sys_show_no_args},
-		{ "netns",	sys_show_no_args},
+		{ NULL,			sys_show_no_args},
+		{ "netns",		sys_show_no_args},
+		{ "privileged-qkey",	sys_show_no_args},
 		{ 0 }
 	};
 
@@ -86,6 +98,17 @@ static int sys_set_netns_cmd(struct rd *rd, bool enable)
 	return rd_sendrecv_msg(rd, seq);
 }
 
+static int sys_set_privileged_qkey_cmd(struct rd *rd, bool enable)
+{
+	uint32_t seq;
+
+	rd_prepare_msg(rd, RDMA_NLDEV_CMD_SYS_SET,
+		       &seq, (NLM_F_REQUEST | NLM_F_ACK));
+	mnl_attr_put_u8(rd->nlh, RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE, enable);
+
+	return rd_sendrecv_msg(rd, seq);
+}
+
 static bool sys_valid_netns_cmd(const char *cmd)
 {
 	int i;
@@ -111,10 +134,28 @@ 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;
+	int ret;
+
+	if (rd_no_arg(rd)) {
+		pr_err("valid options are: { on | off }\n");
+		return -EINVAL;
+	}
+
+	cmd = parse_on_off("privileged-qkey", rd_argv(rd), &ret);
+	if (ret)
+		return -EINVAL;
+
+	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 +165,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 }
 	};
 
diff --git a/rdma/utils.c b/rdma/utils.c
index 8a091c05..09985069 100644
--- a/rdma/utils.c
+++ b/rdma/utils.c
@@ -473,6 +473,7 @@ static const enum mnl_attr_data_type nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
 	[RDMA_NLDEV_ATTR_STAT_AUTO_MODE_MASK] = MNL_TYPE_U32,
 	[RDMA_NLDEV_ATTR_DEV_DIM] = MNL_TYPE_U8,
 	[RDMA_NLDEV_ATTR_RES_RAW] = MNL_TYPE_BINARY,
+	[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE] = MNL_TYPE_U8,
 };
 
 static int rd_attr_check(const struct nlattr *attr, int *typep)
-- 
2.18.1

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

[Petr Machata]

Patrisious Haddad <phaddad@nvidia.com> writes:

> Enrich rdmatool with an option to enable or disable privileged QKEY.
> When enabled, non-privileged users will be allowed to specify a
> controlled QKEY.
>
> By default this parameter is disabled in order to comply with IB spec.
> According to the IB specification rel-1.6, section 3.5.3:
> "QKEYs with the most significant bit set are considered controlled
> QKEYs, and a HCA does not allow a consumer to arbitrarily specify a
> controlled QKEY."
>
> This allows old applications which existed before the kernel commit:
> 0cadb4db79e1 ("RDMA/uverbs: Restrict usage of privileged QKEYs")
> they can use privileged QKEYs without being a privileged user to now
> be able to work again without being privileged granted they turn on this
> parameter.
>
> rdma tool command examples and output.
>
> $ rdma system show
> netns shared privileged-qkey off copy-on-fork on
>
> $ rdma system set privileged-qkey on
>
> $ rdma system show
> netns shared privileged-qkey on copy-on-fork on
>
> Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>

Again, I'm not familiar with the subject matter, but mechanically this
looks OK to me.

Reviewed-by: Petr Machata <petrm@nvidia.com>

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

[David Ahern]

On 10/23/23 5:22 AM, Patrisious Haddad wrote:
> diff --git a/rdma/sys.c b/rdma/sys.c
> index fd785b25..db34cb41 100644
> --- a/rdma/sys.c
> +++ b/rdma/sys.c
> @@ -40,6 +40,17 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data)
>  				   mode_str);
>  	}
>  
> +	if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) {
> +		uint8_t pqkey_mode;
> +
> +		pqkey_mode =
> +			mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]);

just make it mode so it fits on one line.

40 characters for an attribute name .... I will never understand this
fascination with writing a sentence for an attribute name.

> +
> +		print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey",
> +				   "privileged-qkey %s ", pqkey_mode);
> +
> +	}
> +
>  	if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK])
>  		cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]);
>  

keep Petr's reviewed-by tag on the respin.

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

[Patrisious Haddad]

On 10/24/2023 8:02 PM, David Ahern wrote:
> External email: Use caution opening links or attachments
>
>
> On 10/23/23 5:22 AM, Patrisious Haddad wrote:
>> diff --git a/rdma/sys.c b/rdma/sys.c
>> index fd785b25..db34cb41 100644
>> --- a/rdma/sys.c
>> +++ b/rdma/sys.c
>> @@ -40,6 +40,17 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data)
>>                                   mode_str);
>>        }
>>
>> +     if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) {
>> +             uint8_t pqkey_mode;
>> +
>> +             pqkey_mode =
>> +                     mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]);
> just make it mode so it fits on one line.
will do.
>
> 40 characters for an attribute name .... I will never understand this
> fascination with writing a sentence for an attribute name.
me neither, just following the naming convention in 
rdma/include/uapi/rdma/rdma_netlink.h sadly ...
>
>> +
>> +             print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey",
>> +                                "privileged-qkey %s ", pqkey_mode);
>> +
>> +     }
>> +
>>        if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK])
>>                cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]);
>>
> keep Petr's reviewed-by tag on the respin.
will do, just making sure I understand "the respin" , you mean when I 
re-send it ?

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

[Petr Machata]

Patrisious Haddad <phaddad@nvidia.com> writes:

> On 10/24/2023 8:02 PM, David Ahern wrote:
>
>>> +
>>> +             print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey",
>>> +                                "privileged-qkey %s ", pqkey_mode);
>>> +
>>> +     }
>>> +
>>>        if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK])
>>>                cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]);
>>>
>> keep Petr's reviewed-by tag on the respin.
>
> will do, just making sure I understand "the respin" , you mean when I
> re-send it ?

Yes, to send a new version with fixes.

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

[Petr Machata]

David Ahern <dsahern@gmail.com> writes:

> On 10/23/23 5:22 AM, Patrisious Haddad wrote:
>> diff --git a/rdma/sys.c b/rdma/sys.c
>> index fd785b25..db34cb41 100644
>> --- a/rdma/sys.c
>> +++ b/rdma/sys.c
>> @@ -40,6 +40,17 @@ static int sys_show_parse_cb(const struct nlmsghdr *nlh, void *data)
>>  				   mode_str);
>>  	}
>>  
>> +	if (tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]) {
>> +		uint8_t pqkey_mode;
>> +
>> +		pqkey_mode =
>> +			mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE]);
>
> just make it mode so it fits on one line.
>
> 40 characters for an attribute name .... I will never understand this
> fascination with writing a sentence for an attribute name.

Yeah, I noticed this after the review, and figured oh whatever. But this
would be better:

		int at = RDMA_NLDEV_SYS_ATTR_PRIVILEGED_QKEY_MODE;
		uint8_t pqkey_mode;

		pqkey_mode = mnl_attr_get_u8(tb[at]);
		print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey",
				   "privileged-qkey %s ", pqkey_mode);

>> +
>> +		print_color_on_off(PRINT_ANY, COLOR_NONE, "privileged-qkey",
>> +				   "privileged-qkey %s ", pqkey_mode);
>> +
>> +	}
>> +
>>  	if (tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK])
>>  		cof = mnl_attr_get_u8(tb[RDMA_NLDEV_SYS_ATTR_COPY_ON_FORK]);
>>  
>
> keep Petr's reviewed-by tag on the respin.

[PATCH v2 iproute2-next 3/3] rdma: Adjust man page for rdma system set privileged_qkey command

[Patrisious Haddad]

Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
---
 man/man8/rdma-system.8 | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/man/man8/rdma-system.8 b/man/man8/rdma-system.8
index ab1d89fd..a2914eb8 100644
--- a/man/man8/rdma-system.8
+++ b/man/man8/rdma-system.8
@@ -23,16 +23,16 @@ rdma-system \- RDMA subsystem configuration
 
 .ti -8
 .B rdma system set
-.BR netns
-.BR NEWMODE
+.BR netns/privileged_qkey
+.BR NEWMODE/NEWSTATE
 
 .ti -8
 .B rdma system help
 
 .SH "DESCRIPTION"
-.SS rdma system set - set RDMA subsystem network namespace mode
+.SS rdma system set - set RDMA subsystem network namespace mode or privileged qkey mode
 
-.SS rdma system show - display RDMA subsystem network namespace mode
+.SS rdma system show - display RDMA subsystem network namespace mode and privileged qkey state
 
 .PP
 .I "NEWMODE"
@@ -49,12 +49,21 @@ network namespaces is not needed, shared mode can be used.
 
 It is preferred to not change the subsystem mode when there is active
 RDMA traffic running, even though it is supported.
+.PP
+.I "NEWSTATE"
+- specifies the new state of the privileged_qkey parameter. Either enabled or disabled.
+Whereas this decides whether a non-privileged user is allowed to specify a controlled
+QKEY or not, since such QKEYS are considered privileged.
+
+When this parameter is enabled, non-privileged users will be allowed to
+specify a controlled QKEY.
 
 .SH "EXAMPLES"
 .PP
 rdma system show
 .RS 4
-Shows the state of RDMA subsystem network namespace mode on the system.
+Shows the state of RDMA subsystem network namespace mode on the system and
+the state of privileged qkey parameter.
 .RE
 .PP
 rdma system set netns exclusive
@@ -69,6 +78,19 @@ Sets the RDMA subsystem in network namespace shared mode. In this mode RDMA devi
 are shared among network namespaces.
 .RE
 .PP
+.PP
+rdma system set privileged_qkey enabled
+.RS 4
+Sets the privileged_qkey parameter to enabled. In this state non-privileged user
+is allowed to specify a controlled QKEY.
+.RE
+.PP
+rdma system set privileged_qkey disabled
+.RS 4
+Sets the privileged_qkey parameter to disabled. In this state non-privileged user
+is *not* allowed to specify a controlled QKEY.
+.RE
+.PP
 
 .SH SEE ALSO
 .BR rdma (8),
-- 
2.18.1

Re: [PATCH v2 iproute2-next 3/3] rdma: Adjust man page for rdma system set privileged_qkey command

[Petr Machata]

Patrisious Haddad <phaddad@nvidia.com> writes:

> Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
> ---
>  man/man8/rdma-system.8 | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/man/man8/rdma-system.8 b/man/man8/rdma-system.8
> index ab1d89fd..a2914eb8 100644
> --- a/man/man8/rdma-system.8
> +++ b/man/man8/rdma-system.8
> @@ -23,16 +23,16 @@ rdma-system \- RDMA subsystem configuration
>  
>  .ti -8
>  .B rdma system set
> -.BR netns
> -.BR NEWMODE
> +.BR netns/privileged_qkey
> +.BR NEWMODE/NEWSTATE

What is this netns/priveleged_qkey syntax? I thought they are
independent options. If so, the way to express it is:

	rdma system set [netns NEWMODE] [privileged_qkey NEWSTATE]

Also, your option is not actually privileged_qkey, but privileged-qkey.

>  .ti -8
>  .B rdma system help
>  
>  .SH "DESCRIPTION"
> -.SS rdma system set - set RDMA subsystem network namespace mode
> +.SS rdma system set - set RDMA subsystem network namespace mode or privileged qkey mode
>  
> -.SS rdma system show - display RDMA subsystem network namespace mode
> +.SS rdma system show - display RDMA subsystem network namespace mode and privileged qkey state

Maybe make it just something like "configure RDMA system settings" or
whatever the umbrella term is? The next option will certainly have to do
something, this doesn't scale.

Plus the lines are waaay over 80, even over 90 that I think I've seen
Stephen or David mention as OK for iproute2 code.

>  .PP
>  .I "NEWMODE"
> @@ -49,12 +49,21 @@ network namespaces is not needed, shared mode can be used.
>  
>  It is preferred to not change the subsystem mode when there is active
>  RDMA traffic running, even though it is supported.
> +.PP
> +.I "NEWSTATE"
> +- specifies the new state of the privileged_qkey parameter. Either enabled or disabled.
> +Whereas this decides whether a non-privileged user is allowed to specify a controlled
> +QKEY or not, since such QKEYS are considered privileged.
> +
> +When this parameter is enabled, non-privileged users will be allowed to
> +specify a controlled QKEY.

This is missing syntax notes. One might think that to enable it they
need to say "enable", but in fact it's "on", and "off" for disabled.
There should be an "{on | off}" somewhere in there.

Also, line length.

Also, the paragraph is imho a bit long-winded. Maybe make it just this?

	determines whether a non-privileged user is allowed to specify a
        controlled QKEY or not.

>  .SH "EXAMPLES"
>  .PP
>  rdma system show
>  .RS 4
> -Shows the state of RDMA subsystem network namespace mode on the system.
> +Shows the state of RDMA subsystem network namespace mode on the system and
> +the state of privileged qkey parameter.
>  .RE
>  .PP
>  rdma system set netns exclusive
> @@ -69,6 +78,19 @@ Sets the RDMA subsystem in network namespace shared mode. In this mode RDMA devi
>  are shared among network namespaces.
>  .RE
>  .PP
> +.PP
> +rdma system set privileged_qkey enabled
> +.RS 4
> +Sets the privileged_qkey parameter to enabled. In this state non-privileged user
> +is allowed to specify a controlled QKEY.
> +.RE
> +.PP
> +rdma system set privileged_qkey disabled
> +.RS 4
> +Sets the privileged_qkey parameter to disabled. In this state non-privileged user
> +is *not* allowed to specify a controlled QKEY.
> +.RE
> +.PP

on | off, not enabled | disabled.

>  .SH SEE ALSO
>  .BR rdma (8),

Re: [PATCH v2 iproute2-next 3/3] rdma: Adjust man page for rdma system set privileged_qkey command

[David Ahern]

On 10/24/23 10:09 AM, Petr Machata wrote:
> 
> Patrisious Haddad <phaddad@nvidia.com> writes:
> 
>> Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
>> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>> ---
>>  man/man8/rdma-system.8 | 32 +++++++++++++++++++++++++++-----
>>  1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/man/man8/rdma-system.8 b/man/man8/rdma-system.8
>> index ab1d89fd..a2914eb8 100644
>> --- a/man/man8/rdma-system.8
>> +++ b/man/man8/rdma-system.8
>> @@ -23,16 +23,16 @@ rdma-system \- RDMA subsystem configuration
>>  
>>  .ti -8
>>  .B rdma system set
>> -.BR netns
>> -.BR NEWMODE
>> +.BR netns/privileged_qkey
>> +.BR NEWMODE/NEWSTATE
> 
> What is this netns/priveleged_qkey syntax? I thought they are
> independent options. If so, the way to express it is:
> 
> 	rdma system set [netns NEWMODE] [privileged_qkey NEWSTATE]
> 
> Also, your option is not actually privileged_qkey, but privileged-qkey.

yes, and the command lines below show 'privileged qkey'

> 
>>  .ti -8
>>  .B rdma system help
>>  
>>  .SH "DESCRIPTION"
>> -.SS rdma system set - set RDMA subsystem network namespace mode
>> +.SS rdma system set - set RDMA subsystem network namespace mode or privileged qkey mode
>>  
>> -.SS rdma system show - display RDMA subsystem network namespace mode
>> +.SS rdma system show - display RDMA subsystem network namespace mode and privileged qkey state
> 
> Maybe make it just something like "configure RDMA system settings" or
> whatever the umbrella term is? The next option will certainly have to do
> something, this doesn't scale.
> 
> Plus the lines are waaay over 80, even over 90 that I think I've seen
> Stephen or David mention as OK for iproute2 code.

a few over 80 is ok when it improves readability; over 90 (with the
exception of print strings) is unacceptable.

Re: [PATCH v2 iproute2-next 3/3] rdma: Adjust man page for rdma system set privileged_qkey command

[Patrisious Haddad]

On 10/24/2023 7:09 PM, Petr Machata wrote:
> Patrisious Haddad <phaddad@nvidia.com> writes:
>
>> Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
>> Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
>> ---
>>   man/man8/rdma-system.8 | 32 +++++++++++++++++++++++++++-----
>>   1 file changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/man/man8/rdma-system.8 b/man/man8/rdma-system.8
>> index ab1d89fd..a2914eb8 100644
>> --- a/man/man8/rdma-system.8
>> +++ b/man/man8/rdma-system.8
>> @@ -23,16 +23,16 @@ rdma-system \- RDMA subsystem configuration
>>   
>>   .ti -8
>>   .B rdma system set
>> -.BR netns
>> -.BR NEWMODE
>> +.BR netns/privileged_qkey
>> +.BR NEWMODE/NEWSTATE
> What is this netns/priveleged_qkey syntax? I thought they are
> independent options. If so, the way to express it is:
>
> 	rdma system set [netns NEWMODE] [privileged_qkey NEWSTATE]
>
> Also, your option is not actually privileged_qkey, but privileged-qkey.

I'll fix the typo in the argument name, but about them being independent 
while that is true I used or,

since they can't be configured using the same command whereas your 
proposed syntax gives the feeling that you can

configure both in the same rdma system set command.

>
>>   .ti -8
>>   .B rdma system help
>>   
>>   .SH "DESCRIPTION"
>> -.SS rdma system set - set RDMA subsystem network namespace mode
>> +.SS rdma system set - set RDMA subsystem network namespace mode or privileged qkey mode
>>   
>> -.SS rdma system show - display RDMA subsystem network namespace mode
>> +.SS rdma system show - display RDMA subsystem network namespace mode and privileged qkey state
> Maybe make it just something like "configure RDMA system settings" or
> whatever the umbrella term is? The next option will certainly have to do
> something, this doesn't scale.
>
> Plus the lines are waaay over 80, even over 90 that I think I've seen
> Stephen or David mention as OK for iproute2 code.

Will fix all line lengths, thanks for noting.

True about scaling, but isn't the idea behind man-page to be 
comprehensive and list all options explicitly ?

>
>>   .PP
>>   .I "NEWMODE"
>> @@ -49,12 +49,21 @@ network namespaces is not needed, shared mode can be used.
>>   
>>   It is preferred to not change the subsystem mode when there is active
>>   RDMA traffic running, even though it is supported.
>> +.PP
>> +.I "NEWSTATE"
>> +- specifies the new state of the privileged_qkey parameter. Either enabled or disabled.
>> +Whereas this decides whether a non-privileged user is allowed to specify a controlled
>> +QKEY or not, since such QKEYS are considered privileged.
>> +
>> +When this parameter is enabled, non-privileged users will be allowed to
>> +specify a controlled QKEY.
> This is missing syntax notes. One might think that to enable it they
> need to say "enable", but in fact it's "on", and "off" for disabled.
> There should be an "{on | off}" somewhere in there.
>
> Also, line length.
>
> Also, the paragraph is imho a bit long-winded. Maybe make it just this?
>
> 	determines whether a non-privileged user is allowed to specify a
>          controlled QKEY or not.
will rewrite.
>
>>   .SH "EXAMPLES"
>>   .PP
>>   rdma system show
>>   .RS 4
>> -Shows the state of RDMA subsystem network namespace mode on the system.
>> +Shows the state of RDMA subsystem network namespace mode on the system and
>> +the state of privileged qkey parameter.
>>   .RE
>>   .PP
>>   rdma system set netns exclusive
>> @@ -69,6 +78,19 @@ Sets the RDMA subsystem in network namespace shared mode. In this mode RDMA devi
>>   are shared among network namespaces.
>>   .RE
>>   .PP
>> +.PP
>> +rdma system set privileged_qkey enabled
>> +.RS 4
>> +Sets the privileged_qkey parameter to enabled. In this state non-privileged user
>> +is allowed to specify a controlled QKEY.
>> +.RE
>> +.PP
>> +rdma system set privileged_qkey disabled
>> +.RS 4
>> +Sets the privileged_qkey parameter to disabled. In this state non-privileged user
>> +is *not* allowed to specify a controlled QKEY.
>> +.RE
>> +.PP
> on | off, not enabled | disabled.
yeah I don't know how I missed that will fix in all instances of man-page.
>
>>   .SH SEE ALSO
>>   .BR rdma (8),