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@xxxxxxxxxx> writes:
>
> > Signed-off-by: Patrisious Haddad <phaddad@xxxxxxxxxx>
> > Reviewed-by: Michael Guralnik <michaelgur@xxxxxxxxxx>
> > ---
> >   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),