Date: Wed, 25 Oct 2023 09:10:46 +0300
From: Patrisious Haddad <phaddad@...dia.com>
To: Petr Machata <petrm@...dia.com>
Cc: jgg@...pe.ca, leon@...nel.org, dsahern@...il.com,
 stephen@...workplumber.org, netdev@...r.kernel.org,
 linux-rdma@...r.kernel.org, linuxarm@...wei.com,
 linux-kernel@...r.kernel.org, huangjunxian6@...ilicon.com,
 michaelgur@...dia.com
Subject: Re: [PATCH v2 iproute2-next 3/3] rdma: Adjust man page for rdma
 system set privileged_qkey command


On 10/24/2023 7:09 PM, Petr Machata wrote:
> Patrisious Haddad <phaddad@...dia.com> writes:
>
>> Signed-off-by: Patrisious Haddad <phaddad@...dia.com>
>> Reviewed-by: Michael Guralnik <michaelgur@...dia.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),

Powered by blists - more mailing lists