Date:   Tue, 10 Jan 2023 20:28:35 +0100
From:   Alejandro Colomar <alx.manpages@...il.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        linux-man@...r.kernel.org, Alejandro Colomar <alx@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Boqun Feng <boqun.feng@...il.com>, paulmck <paulmck@...nel.org>
Subject: Re: rseq(2) man page



On 1/10/23 17:54, Mathieu Desnoyers wrote:

[...]

[EFAULT / SEGV]

> 
> OK, let's keep this for a separate discussion.

Sure.

[...]

>> I forgot; there's a new section: LIBRARY.
>>
>> It specifies the library in which the function is defined, or in the case of 
>> syscalls, the wrapper (since we call it through syscall(2), it would be libc).
> 
> OK. Similar to futex(2).

Yep.

> 
>>
>> BTW, I wonder what librseq is.  Is librseq something that users should care 
>> about?
> 
> Users are not required to use librseq to use the rseq system call,
> but it's very convenient to use this C-level API rather than have
> each user reimplement the per-architecture assembly code required to
> create the RSEQ critical sections.
> 
> librseq did not have an official release yet, so that's mainly why I
> think it too early to refer to it in manual pages.

Okay; when you feel it's ready, you could document it like libkeyutils in keyctl(2).

> 
>>
>>> .SH SYNOPSIS
>>> .nf
>>> .PP
>>> .BR "#include <linux/rseq.h>" \
>>> " /* Definition of " RSEQ_* " constants and rseq types */"
>>
>> The line above goes beyond column 80 in formatted output.  That's a hard limit 
>> for manual pages.  If you add this page to the linux man-pages repo, and run 
>> the linters, you'll see a warning about that.  In case you're interested in 
>> linting manual pages in the future, you can do something similar to what I do 
>> in the man-pages[2]
> 
> OK. I've used it, it's quite useful! I have fixed all warnings except for
> "mandoc: man2/rseq.2:5:12: WARNING: cannot parse date, using it verbatim: (date)"
> which I suspect is expected.

Uhh, I had that one explicitly silenced:

$ make lint-man-mandoc V=1
LINT (mandoc)	tmp/lint/man2/perf_event_open.2.lint-man.mandoc.touch
! (mandoc -man -Tlint  man2/perf_event_open.2 2>&1 \
    | grep -v 'STYLE: lower case character in document title:' \
    | grep -v 'UNSUPP: ignoring macro in table:' \
    | grep -v 'WARNING: cannot parse date, using it verbatim: TH (date)' \
    | grep -v 'WARNING: empty block: UR' \
    ||:; \
) \
| grep '.' >&2
touch tmp/lint/man2/perf_event_open.2.lint-man.mandoc.touch


For some reason you didn't have "TH" in the error message, which is why my grep 
didn't discard it.

[...]

>>> .BI "int syscall(SYS_rseq, struct rseq *_Nullable " rseq ", uint32_t " 
>>> rseq_len \
>>
>> What's the meaning for NULL?  Does it have a valid sentinel meaning, or is it 
>> an invalid address?  If it's just interpreted as an invalid address (for which 
>> from a user-space perspective a crash would be legitimate), then I'd remove 
>> _Nullable.
> 
> With the flags that are currently implemented (0 or RSEQ_FLAG_UNREGISTER),
> the rseq argument is not expected to be legitimately NULL (it would return
> -1, errno=EFAULT on registration, or -1, errno=EINVAL on unregister attempt).
> 
> We may add new flags in the future which would not care about the rseq address
> (it could very well be null then). Do you recommend that we only add the
> _Nullable tag when this occurs ?

Yes; since it's what the user can pass, it makes sense to be as constrained as 
possible.  If it were some return that the user would have to inspect, it would 
make sense to be cautious on the NULL side of things an use _Nullable 
preventively, but for an input, non-null is preferred for now.


[...]

> 
> Updated version based on your comments pushed into my repo, thanks!

Cool! I'll have a look.

Cheers,

Alex

> 
> Mathieu
> 
> 

-- 
<http://www.alejandro-colomar.es/>

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists