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).
It isn't absolutely required. On the one hand, people can use
"man -k any=SA_RESTART" to find out what it is. On the other hand,
people can maybe be expected to know that sigaction(2) is the system
call to configure signal handling, and even if they ognore it, they
will easily find out with "man signal".
Then again, i can see how saying more precisely what SA_RESTART is
might be helpful especially for novice users, if it can be done
without being too much of a distraction.
In general, i dislike both parenthetic remarks and "see ... for details"
unless a more natural wording cannot be found.
Maybe something like:
... even if SA_RESTART was set with sigaction(2) for the
interrupting signal.
Your call, i would say.
> 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.
> 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.
> 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.
> 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".
> 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.
> 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.
See below in-line for some nits. Getting rid of \\ is mandatory;
the rest are only suggestions.
Yours,
Ingo
> 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.
> +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 ,
> +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.
> +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 EXAMPLES
> +Count down from ten:
> +.Bd -literal
> + struct timespec one_second;
> + int i;
> +
> + one_second.tv_sec = 1;
> + one_second.tv_nsec = 0;
> +
> + for (i = 10; i >= 0; i--) {
> + if (nanosleep(&one_second, NULL) == -1)
> + err(1, "nanosleep");
> + printf("%4d\\n", i);
This must be \en, not \\n.
Using \\ outside macro definitions (i.e., outside the bodies of
roff(7) .de and similar requests) is almost always wrong even in
low-level roff code, and \\ must never be used in manual pages.
> + }
> +.Ed
> +.Pp
> +Check a condition once every one hundred milliseconds:
> +.Bd -literal
> + struct timespec hundred_ms;
> + extern int done;
> +
> + hundred_ms.tv_sec = 0;
> + hundred_ms.tv_nsec = 100 * 1000 * 1000;
> +
> + while (!done) {
> + if (nanosleep(&hundred_ms, NULL) == -1)
> + err(1, "nanosleep");
> + }
> +.Ed
> +.Pp
> +Sleep for at least sixty seconds and print the time remaining whenever
> +a signal interrupts the sleep:
> +.Bd -literal
> + struct timespec timeout;
> +
> + timeout.tv_sec = 60;
> + timeout.tv_nsec = 0;
> +
> + while (nanosleep(&timeout, &timeout) == -1) {
> + if (errno != EINTR)
> + err(1, "nanosleep");
> + printf("time left: %lld.%09ld seconds\\n",
Again, \en.
> + (long long)timeout.tv_sec, timeout.tv_nsec);
> + }
> +.Ed
> +.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 was 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,
> +one billion,
> or a second value less than zero.
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.
> .El
> .Sh SEE ALSO
> .Xr sleep 1 ,
> +.Xr sigaction 2 ,
> .Xr sleep 3
> .Sh STANDARDS
> The
> @@ -97,17 +158,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 .
