On Wed, Jul 21, 2021 at 04:00:51AM +0200, Ingo Schwarze wrote: > Hi Scott, > > Scott Cheloha wrote on Tue, Jul 20, 2021 at 05:20:16PM -0500: > > > The nanosleep.2 page could use some cleanup. Here's a bunch of fixes, > > rewrites, etc. > > > > I've included my notes on the changes below. I have some (mostly > > stylistic) questions in there, too. > > Thanks for explaining what you are doing. I'm snipping what i agree > with as well as what i have no opinion on and trust you on, but that > doesn't mean that mentioning it was useless. > > > Should we reference sigaction(2) here alongside SA_RESTART? e.g.: > > > > ... even if SA_RESTART is set for the interrupting signal > > (see sigaction(2) for further details). > > [...] > > Maybe something like: > > ... even if SA_RESTART was set with sigaction(2) for the > interrupting signal. > > Your call, i would say.
I like that. Using this snippet now. > > I'm unsure about the correct voice here. Should nanosleep actively > > return values? Or should values "be returned"? > > Both are acceptable. In general, and not just in manual pages, > prefer the active voice when both work equally well. Staying active, then. > > I'm unclear on whether I need the indent here in the .Bd block. I > > think it looks better than unindented code, but maybe I'm flouting > > some practice we have. > > Using tabs is permitted inside .Bd -literal and .Bd -unfilled. > They should definitely be used for indenting the bodies of for, > while, and if. > > Whether you indent the whole thing depends on what looks better. > When the code is relatively narrow, indenting the whole example > is often a good idea. When the code contains very long lines, > not indenting the whole example may be better. > > If you choose to indent, i have no strong prefernce whether you > do it with tabs or with .Bd -literal -offset indent; maybe i very > slightly prefer the latter, because the global indentation is > a formatting choice rather than part of the displayed code. > But i really wouldn't complain about either. deraadt@ has vetoed the examples so this is no longer an issue. > > ERRORS > > - Simplify the opening sentence. Yeesh, what a mouthful. > > Indeed, ERRORS should not repeat the content of RETURN VALUES. > > > Unsure if a second EFAULT case for remainder is a good thing here. > > Strictly speaking, NULL is not a valid part of the process address > > space. Maybe that's too pedantic? > > I don't think you can be too pedantic about where NULL is allowed > and where it isn't. That's a notorious source of bugs, so precision > about NULL is usually a good thing. Keeping it, then. > > Also, do we have a more standard "boilerplate" sentence for EFAULT? > > Judging from "man -l /usr/share/man/man2/*.2", the most common > wording is: > > [EFAULT] foo points outside the process's allocated address space. > > But i don't really i like that. The word "allocated" makes me wonder > because it sounds too much like malloc(3) for my taste. > Usually, pointers to automatic and to static objects are acceptable, > too, and are those "allocated"? Some might say they are not. > Besides, "process's" looks awkward. > > These occur, too: > > [EFAULT] The foo parameter is not in a valid part of the user > address space. > > [EFAULT] foo references invalid memory. > > [EFAULT] The name parameter specifies an area outside the > process address space. > > [EFAULT] foo points to an illegal address. > > [EFAULT] The userspace address uaddr is invalid. > > [EFAULT] Part of buf points outside the process's allocated > address space. > > [EFAULT] The buf parameter points to an invalid address. > > [EFAULT] The argument foo specifies an invalid address. > > [EFAULT] The foo parameter specified a bad address. > > [EFAULT] The foo parameter points to memory not in > a valid part of the process address space. > > [EFAULT] The address specified for foo is invalid. > > [EFAULT] An argument address referenced invalid memory. > > In errno(2): > 14 EFAULT Bad address. The system detected an invalid address in > attempting to use an argument of a call. > > [EFAULT] There was an error reading or writing the kevent > structure. > > Quite a mess, i would say. > > I think trying to explain over and over again what an invalid address > is, in each and every manual page, is neither reasonable nor feasible - > i'm not convinced the rules are really simple given how many types > of valid memory there are: static, stack, heap, ... > > So i'd probably settle for a concise, simple wording, and i think > i like this one best: > > [EFAULT] foo points to an invalid address. > > What is valid can easily depend on the function being called and > even on other function arguments; the same address can easily be > valid for reading a handful of bytes but invalid for reading > kilobytes, let alone megabytes of data. > > Please don't say "illegal" (it's not a crime) nor "bad". Thank you for diving into this. Given deraadt@'s response I'm just going to leave the existing language. Somebody else can unify the language for all the EFAULT cases as a future summer project, I guess. > > SEE ALSO > > - Cross reference sigaction(2). We mention SA_RESTART in the > > description. > > Yes, that seems helpful. > > Users of nanosleep(2) *should* consider signal handling, and sigaction(2) > is the canonical place to learn about that. > > > - I'm pretty sure nanosleep() is a POSIX invention, so note that it > > really first appeared in POSIX.1b (realtime extensions), > > That would be quite unusual; as a rule, the Austin group categorically > refuses to standardize anything that doesn't at least have two > implementations in relevant real-world operating systems. > Consequently, we almost never have .St below HISTORY. > > Then again, i believe you know this stuff, so i trust you if you > say POSIX invented this and did not merely adopt it from some > commercial UNIX system. And if that is true, if standardization > really predates implementation in this case, then using .St > below HISTORY is indeed the right thing to do. I guess I will need to dig into it a bit. Finding the text of the really early documents, prior to SUSv2, is tricky. > > If someone can find nanosleep() in the wild before 1993 we can fix > > the attribution. > > Fair enough. You can also ask millert@ if you have doubts, he often > remembers details about POSIX that many others don't. Sure thing. > See below in-line for some nits. Getting rid of \\ is mandatory; > the rest are only suggestions. > > [...] > > > Index: nanosleep.2 > > =================================================================== > > RCS file: /cvs/src/lib/libc/sys/nanosleep.2,v > > retrieving revision 1.16 > > diff -u -p -r1.16 nanosleep.2 > > --- nanosleep.2 31 Dec 2018 18:54:00 -0000 1.16 > > +++ nanosleep.2 20 Jul 2021 22:14:50 -0000 > > @@ -41,53 +41,114 @@ > > .Ft int > > .Fn nanosleep "const struct timespec *timeout" "struct timespec *remainder" > > .Sh DESCRIPTION > > +The > > .Fn nanosleep > > -suspends execution of the calling process for the duration specified by > > +function suspends execution of the calling thread for at least the > > +duration specified by > > .Fa timeout . > > -An unmasked signal will cause it to terminate the sleep early, > > -regardless of the > > +Delivery of an unmasked signal to the calling thread will terminate the > > s/will terminate/terminates/ is enough; "will" is needed rarely > in manual pages. Changed. > > +sleep early, > > +even if > > .Dv SA_RESTART > > -value on the interrupting signal. > > +is set for the interrupting signal. > > .Sh RETURN VALUES > > -If the > > +If > > .Fn nanosleep > > -function returns because the requested duration has elapsed, the value > > -returned will be zero. > > +sleeps the full > > +.Fa timeout > > +without interruption it returns 0. > > +If > > +.Fa remainder > > +is > > +.Pf non- Dv NULL > > Prefixing non- like this is acceptable, but not particularly nice. > I would like this better: > > Unless > .Fa remainder > is > .Dv NULL , Changed. > > +it is set to zero. > > .Pp > > -If the > > +If > > .Fn nanosleep > > -function returns due to the delivery of a signal, the value returned > > -will be \-1, and the global variable > > +is interrupted by a signal it returns -1 and the global variable > > .Va errno > > -will be set to indicate the interruption. > > +is set to > > +.Dv EINTR . > > If > > .Fa remainder > > -is non-null, the timespec structure it references is updated to contain the > > -unslept amount (the requested duration minus the duration actually slept). > > -.Sh ERRORS > > -If any of the following conditions occur, the > > +is > > +.Pf non- Dv NULL > > Unless... as above. Changed. > [...] > > Regarding the following, why not be very explicit, considering that > the rules differ for both cases? > > > .It Bq Er EFAULT > > -Either > > .Fa timeout > > -or > > +pointed to memory that is not a valid part of the process address space. > > .Fa timeout > was > .Dv NULL > or pointed to an invalid address. > > > +.It Bq Er EFAULT > > .Fa remainder > > -points to memory that is not a valid part of the process address space. > > +was > > +.Pf non- Dv NULL > > +and pointed to memory that is not a valid part of the process address > > space. > > .Fa remainder > was not > .Dv NULL > and pointed to an invalid address. As I said before, I'm just going to keep the existing language. -- Okay, updated patch attached. One additional change: - Escape the minus sign for "-1". Index: nanosleep.2 =================================================================== RCS file: /cvs/src/lib/libc/sys/nanosleep.2,v retrieving revision 1.16 diff -u -p -r1.16 nanosleep.2 --- nanosleep.2 31 Dec 2018 18:54:00 -0000 1.16 +++ nanosleep.2 21 Jul 2021 15:57:55 -0000 @@ -41,53 +41,71 @@ .Ft int .Fn nanosleep "const struct timespec *timeout" "struct timespec *remainder" .Sh DESCRIPTION +The .Fn nanosleep -suspends execution of the calling process for the duration specified by +function suspends execution of the calling thread for at least the +duration specified by .Fa timeout . -An unmasked signal will cause it to terminate the sleep early, -regardless of the +Delivery of an unmasked signal to the calling thread terminates the +sleep early, +even if .Dv SA_RESTART -value on the interrupting signal. +is set with +.Xr sigaction 2 +for the interrupting signal. .Sh RETURN VALUES -If the +If .Fn nanosleep -function returns because the requested duration has elapsed, the value -returned will be zero. +sleeps the full +.Fa timeout +without interruption it returns 0. +Unless +.Fa remainder +is +.Dv NULL , +it is set to zero. .Pp -If the +If .Fn nanosleep -function returns due to the delivery of a signal, the value returned -will be \-1, and the global variable +is interrupted by a signal it returns \-1 and the global variable .Va errno -will be set to indicate the interruption. -If +is set to +.Dv EINTR . +Unless .Fa remainder -is non-null, the timespec structure it references is updated to contain the -unslept amount (the requested duration minus the duration actually slept). -.Sh ERRORS -If any of the following conditions occur, the +is +.Dv NULL , +it is set to the unslept portion of the +.Fa timeout . +.Pp +Otherwise, .Fn nanosleep -function shall return \-1 and set +returns \-1 and the global variable .Va errno -to the corresponding value. +is set to indicate the error. +.Sh ERRORS +.Fn nanosleep +will fail if: .Bl -tag -width Er .It Bq Er EINTR -.Fn nanosleep -was interrupted by the delivery of a signal. +The call is interrupted by the delivery of a signal. .It Bq Er EINVAL .Fa timeout -specified a nanosecond value less than zero or greater than or equal to -1000 million, +specifies a nanosecond value less than zero or greater than or equal to +one billion, or a second value less than zero. .It Bq Er EFAULT -Either .Fa timeout -or -.Fa remainder points to memory that is not a valid part of the process address space. +.It Bq Er EFAULT +.Fa remainder +is not +.Dv NULL +and points to memory that is not a valid part of the process address space. .El .Sh SEE ALSO .Xr sleep 1 , +.Xr sigaction 2 , .Xr sleep 3 .Sh STANDARDS The @@ -97,17 +115,23 @@ function conforms to .Sh HISTORY The predecessor of this system call, .Fn sleep , -appeared in -.At v3 , -but was removed when +first appeared in +.At v3 . +It was removed in +.At v7 +and replaced with a C library implementation based on .Xr alarm 3 -was introduced into -.At v7 . +and +.Xr signal 3 . +.Pp The .Fn nanosleep -system call has been available since +function first appeared in +.St -p1003.1b-93 . +.Pp +This implementation of +.Fn nanosleep +first appeared in .Nx 1.3 and was ported to -.Ox 2.1 -and -.Fx 3.0 . +.Ox 2.1 .
