[PATCH] Fix the man page

[PATCH] Fix the man page

[Zijun Zhao]

Hi there,
  We are annotating settimeofday(), gettimeofday() and we will make tv
Nonnull if compilation warnings will

result. But after checking
https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L199
nd https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L224,

we think tv is okay to be nullable. So we make the fix to make it more clear.

Best,

Zijun Zhao

Re: [PATCH] Fix the man page

[Zijun Zhao]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

Hi there,
  We are annotating settimeofday(), gettimeofday() and we will make tv
Nonnull if compilation warnings will

result. But after checking
https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L199
nd https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L224,

we think tv is okay to be nullable. So we make the fix to make it more clear.

Best,

Zijun Zhao

[-- Attachment #2: fix.patch --]
[-- Type: application/octet-stream, Size: 1066 bytes --]

From 760d8e07c8b15cfcb88de9f064db8cae577f7ac7 Mon Sep 17 00:00:00 2001
From: Zijun Zhao <zijunzhao@google.com>
Date: Fri, 12 May 2023 19:03:47 -0700
Subject: [PATCH] Fix the man page

We are annotating settimeofday() and we will make tv Nonnull if compilation warnings will
result. But after checking https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L199,
we think it is okay to be nullable. So we make the fix to make it more clear.
---
 man2/gettimeofday.2 | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/man2/gettimeofday.2 b/man2/gettimeofday.2
index 9d134fa49..1e71c5b70 100644
--- a/man2/gettimeofday.2
+++ b/man2/gettimeofday.2
@@ -92,9 +92,6 @@ or
 is NULL, the corresponding structure is not set or returned.
 .\" FIXME . The compilation warning looks to be going away in glibc 2.17
 .\" see glibc commit 4b7634a5e03b0da6f8875de9d3f74c1cf6f2a6e8
-(However, compilation warnings will result if
-.I tv
-is NULL.)
 .\" The following is covered under EPERM below:
 .\" .PP
 .\" Only the superuser may use
-- 
2.40.1.606.ga4b1b128d6-goog

Re: [PATCH] Fix the man page

[enh]

should probably remove the "The compilation warning looks to be going
away in glibc 2.17 see glibc commit
4b7634a5e03b0da6f8875de9d3f74c1cf6f2a6e8" above, since this patch
fixes that, but leave the FIXME because it looks like there are more
FIXMEs below?


On Thu, May 18, 2023 at 4:27 PM Zijun Zhao <zijunzhao@google.com> wrote:
>
> Hi there,
>   We are annotating settimeofday(), gettimeofday() and we will make tv
> Nonnull if compilation warnings will
>
> result. But after checking
> https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L199
> nd https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L224,
>
> we think tv is okay to be nullable. So we make the fix to make it more clear.
>
> Best,
>
> Zijun Zhao

Re: [PATCH] Fix the man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2082 bytes --]

Hi Zijun, Elliott,

On 5/19/23 01:30, enh wrote:
> should probably remove the "The compilation warning looks to be going
> away in glibc 2.17 see glibc commit
> 4b7634a5e03b0da6f8875de9d3f74c1cf6f2a6e8" above, since this patch
> fixes that, but leave the FIXME because it looks like there are more
> FIXMEs below?
> 
> 
> On Thu, May 18, 2023 at 4:27 PM Zijun Zhao <zijunzhao@google.com> wrote:
>>
>> Hi there,
>>    We are annotating settimeofday(), gettimeofday() and we will make tv
>> Nonnull if compilation warnings will
>>
>> result. But after checking
>> https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L199
>> nd https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L224,
>>
>> we think tv is okay to be nullable. So we make the fix to make it more clear.
>>
>> Best,
>>
>> Zijun Zhao

You'll also need to add _Nullable to the function prototypes in the
SYNOPSIS.


BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
only settimeofday(3) that is nullable.  See:


$ grepc gettimeofday /usr/include/*/sys/time.h
/usr/include/x86_64-linux-gnu/sys/time.h:67:
extern int gettimeofday (struct timeval *__restrict __tv,
			 void *__restrict __tz) __THROW __nonnull ((1));


/usr/include/x86_64-linux-gnu/sys/time.h:75:
#  define gettimeofday __gettimeofday64


$ grepc settimeofday /usr/include/*/sys/time.h
/usr/include/x86_64-linux-gnu/sys/time.h:86:
extern int settimeofday (const struct timeval *__tv,
			 const struct timezone *__tz)
      __THROW;


/usr/include/x86_64-linux-gnu/sys/time.h:106:
#   define settimeofday __settimeofday64


And while NULL may be non-UB, the manual page is not very clear on why
someone would want to call these functions with NULL.  Could you
please also explain why would someone want to call these functions
with NULL?  (Let's discuss it in the list, and then we see what
wording we use for the page.)

Thanks!
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix the man page

[enh]

On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar
<alx.manpages@gmail.com> wrote:
>
> Hi Zijun, Elliott,
>
> On 5/19/23 01:30, enh wrote:
> > should probably remove the "The compilation warning looks to be going
> > away in glibc 2.17 see glibc commit
> > 4b7634a5e03b0da6f8875de9d3f74c1cf6f2a6e8" above, since this patch
> > fixes that, but leave the FIXME because it looks like there are more
> > FIXMEs below?
> >
> >
> > On Thu, May 18, 2023 at 4:27 PM Zijun Zhao <zijunzhao@google.com> wrote:
> >>
> >> Hi there,
> >>    We are annotating settimeofday(), gettimeofday() and we will make tv
> >> Nonnull if compilation warnings will
> >>
> >> result. But after checking
> >> https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L199
> >> nd https://elixir.bootlin.com/linux/latest/source/kernel/time/time.c#L224,
> >>
> >> we think tv is okay to be nullable. So we make the fix to make it more clear.
> >>
> >> Best,
> >>
> >> Zijun Zhao
>
> You'll also need to add _Nullable to the function prototypes in the
> SYNOPSIS.

heh, funnily enough, zijunzhao's adding exactly those annotations to
the bionic headers, which is how we stumbled across this.

> BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
> only settimeofday(3) that is nullable.

yeah, but the _kernel_ allows null in both (see kernel/time/time.c),
and this is section 2 of the man page. it's unclear to me whether
that's worth calling out up here, or should be down in the
Linux-specific section? (this page seems to be written as if it cares
about Hurd? "On a non-Linux kernel, with glibc ...".)

> See:
>
>
> $ grepc gettimeofday /usr/include/*/sys/time.h
> /usr/include/x86_64-linux-gnu/sys/time.h:67:
> extern int gettimeofday (struct timeval *__restrict __tv,
>                          void *__restrict __tz) __THROW __nonnull ((1));
>
>
> /usr/include/x86_64-linux-gnu/sys/time.h:75:
> #  define gettimeofday __gettimeofday64
>
>
> $ grepc settimeofday /usr/include/*/sys/time.h
> /usr/include/x86_64-linux-gnu/sys/time.h:86:
> extern int settimeofday (const struct timeval *__tv,
>                          const struct timezone *__tz)
>       __THROW;
>
>
> /usr/include/x86_64-linux-gnu/sys/time.h:106:
> #   define settimeofday __settimeofday64
>
>
> And while NULL may be non-UB, the manual page is not very clear on why
> someone would want to call these functions with NULL.  Could you
> please also explain why would someone want to call these functions
> with NULL?  (Let's discuss it in the list, and then we see what
> wording we use for the page.)

so there are definitely several orders of magnitude more users of a
null timezone --- most callers don't want that (not least because
glibc seems to not fill it out anyway? musl certainly doesn't,
implementing gettimeofday() in terms of clock_gettime() instead).

the users for a null time (and non-null timezone; i haven't seen
anyone -- not even a test -- pass null for both!) are interesting.
Android's init uses settimeofday() with only a timezone, but --
although the timezone to use is settable in device-specific init
scripts -- for devices in our tree, that's actually always 0. there's
other proprietary (non-Google) code that uses gettimeofday() with only
a timezone, and i'm honestly not sure whether it's deliberate that
it's asking the _kernel_ for the timezone or not (i'm guessing they
want the device timezone rather than the app timezone, but i do not
believe there are devices where the _kernel_ timezone matches the
device timezone?). certainly both of these use cases seem worth
following up on, and i will do.

but looking at musl (which ignores tz in both cases) it seems like the
most impactful change would be to fix "The functions gettimeofday()
and settimeofday() can get and set the time as well as a timezone" :-)

it also seems like an improvement to really call out the libc/kernel
differences more clearly though.

> Thanks!
> Alex
>
> --
> <http://www.alejandro-colomar.es/>
> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
>

Re: [PATCH] Fix the man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 7521 bytes --]

On 5/20/23 00:21, enh wrote:
> On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar
> <alx.manpages@gmail.com> wrote:
[...]

>> You'll also need to add _Nullable to the function prototypes in the
>> SYNOPSIS.
> 
> heh, funnily enough, zijunzhao's adding exactly those annotations to
> the bionic headers, which is how we stumbled across this.

:)

> 
>> BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
>> only settimeofday(3) that is nullable.
> 
> yeah, but the _kernel_ allows null in both (see kernel/time/time.c),
> and this is section 2 of the man page. it's unclear to me whether
> that's worth calling out up here, or should be down in the
> Linux-specific section? (this page seems to be written as if it cares
> about Hurd? "On a non-Linux kernel, with glibc ...".)

Yes, we care about hurd, and non-g libc, in this page and others.
Normally, I would be cautious, and specify the strictest prototype of
all implementations about which I care, but in this case, glibc seems
unnecessarily restrictive, and I'm ready to add _Nullable if the kernel
and some libc support that.  However, as a caution, please add a
(CAVEATS?) section documenting that glibc doesn't allow NULL there.
Or maybe keep the SYNOPSIS without _Nullable, and have some VERSIONS
section specifying that other libcs and the kernel support NULL.

> 
>> See:
>>
>>
>> $ grepc gettimeofday /usr/include/*/sys/time.h
>> /usr/include/x86_64-linux-gnu/sys/time.h:67:
>> extern int gettimeofday (struct timeval *__restrict __tv,
>>                          void *__restrict __tz) __THROW __nonnull ((1));
>>
>>
>> /usr/include/x86_64-linux-gnu/sys/time.h:75:
>> #  define gettimeofday __gettimeofday64
>>
>>
>> $ grepc settimeofday /usr/include/*/sys/time.h
>> /usr/include/x86_64-linux-gnu/sys/time.h:86:
>> extern int settimeofday (const struct timeval *__tv,
>>                          const struct timezone *__tz)
>>       __THROW;
>>
>>
>> /usr/include/x86_64-linux-gnu/sys/time.h:106:
>> #   define settimeofday __settimeofday64
>>
>>
>> And while NULL may be non-UB, the manual page is not very clear on why
>> someone would want to call these functions with NULL.  Could you
>> please also explain why would someone want to call these functions
>> with NULL?  (Let's discuss it in the list, and then we see what
>> wording we use for the page.)
> 
> so there are definitely several orders of magnitude more users of a
> null timezone --- most callers don't want that (not least because
> glibc seems to not fill it out anyway? musl certainly doesn't,
> implementing gettimeofday() in terms of clock_gettime() instead).
> 
> the users for a null time (and non-null timezone; i haven't seen
> anyone -- not even a test -- pass null for both!) are interesting.
> Android's init uses settimeofday() with only a timezone, but --
> although the timezone to use is settable in device-specific init
> scripts -- for devices in our tree, that's actually always 0. there's
> other proprietary (non-Google) code that uses gettimeofday() with only
> a timezone, and i'm honestly not sure whether it's deliberate that
> it's asking the _kernel_ for the timezone or not (i'm guessing they
> want the device timezone rather than the app timezone, but i do not
> believe there are devices where the _kernel_ timezone matches the
> device timezone?). certainly both of these use cases seem worth
> following up on, and i will do.

Please.  And thanks for the details!

> 
> but looking at musl (which ignores tz in both cases) it seems like the
> most impactful change would be to fix "The functions gettimeofday()
> and settimeofday() can get and set the time as well as a timezone" :-)

:-)

Makes sense; although it would be worth being clear about if that's
really true, and if libcs do support that as well as the kernel.

> 
> it also seems like an improvement to really call out the libc/kernel
> differences more clearly though.

Sure.  Many pages have a dedicated subsection.  Feel free to add it
to this page.  I'd put it in .SH VERSIONS.

$ grep -rn 'C library/kernel differences' man*
man2/sigprocmask.2:120:.SS C library/kernel differences
man2/setreuid.2:168:.SS C library/kernel differences
man2/brk.2:126:.SS C library/kernel differences
man2/sigwaitinfo.2:114:.SS C library/kernel differences
man2/gethostname.2:122:.SS C library/kernel differences
man2/getcpu.2:73:.SS C library/kernel differences
man2/fork.2:263:.SS C library/kernel differences
man2/epoll_wait.2:258:.SS C library/kernel differences
man2/wait4.2:157:.SS C library/kernel differences
man2/stat.2:355:.SS C library/kernel differences
man2/sigsuspend.2:74:.SS C library/kernel differences
man2/access.2:40:                /* But see C library/kernel differences, below */
man2/access.2:285:.SS C library/kernel differences
man2/clock_getres.2:354:.SS C library/kernel differences
man2/getpid.2:57:.SS C library/kernel differences
man2/setfsgid.2:57:.SS C library/kernel differences
man2/getgroups.2:142:.SS C library/kernel differences
man2/sigaction.2:946:.SS C library/kernel differences
man2/setresuid.2:100:.SS C library/kernel differences
man2/readv.2:290:.SS C library/kernel differences
man2/readv.2:342:.SS Historical C library/kernel differences
man2/open.2:1393:.SS C library/kernel differences
man2/setgid.2:58:.SS C library/kernel differences
man2/setfsuid.2:97:.SS C library/kernel differences
man2/ptrace.2:2814:.SS C library/kernel differences
man2/poll.2:382:.SS C library/kernel differences
man2/sigreturn.2:130:.SS C library/kernel differences
man2/eventfd.2:267:.SS C library/kernel differences
man2/mmap.2:729:.SS C library/kernel differences
man2/_exit.2:104:.SS C library/kernel differences
man2/time.2:86:.SS C library/kernel differences
man2/pread.2:101:.SS C library/kernel differences
man2/seccomp.2:975:.IR "C library/kernel differences" .
man2/setuid.2:104:.SS C library/kernel differences
man2/sched_setaffinity.2:95:return 0 (but see "C library/kernel differences" below,
man2/sched_setaffinity.2:220:.SS C library/kernel differences
man2/select.2:559:.SS C library/kernel differences
man2/sigpending.2:52:.SS C library/kernel differences
man2/posix_fadvise.2:135:.SS C library/kernel differences
man2/gettimeofday.2:173:.SS C library/kernel differences
man2/uname.2:83:.SS C library/kernel differences
man2/clone.2:1576:.SS C library/kernel differences
man2/getpriority.2:185:.SS C library/kernel differences
man2/signalfd.2:331:.SS C library/kernel differences
man2/timer_create.2:225:.SS C library/kernel differences
man2/wait.2:464:.SS C library/kernel differences
man2/nice.2:80:.SS C library/kernel differences
man2/chmod.2:303:.SS C library/kernel differences
man2/seteuid.2:114:.SS C library/kernel differences
man3/getcwd.3:217:.SS C library/kernel differences
man3/killpg.3:98:.SS C library/kernel differences
man3/mq_notify.3:174:.SS C library/kernel differences
man3/mq_open.3:269:.SS C library/kernel differences
man3/sigqueue.3:118:.SS C library/kernel differences
man3type/epoll_event.3type:37:.SS C library/kernel differences
man7/man-pages.7:402:.I "C library/kernel differences"

Cheers,
Alex

> 
>> Thanks!
>> Alex
>>
>> --
>> <http://www.alejandro-colomar.es/>
>> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
>>

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix the man page

[Zijun Zhao]

[-- Attachment #1: Type: text/plain, Size: 8148 bytes --]

Hi Alejandro,
  I already created the patch attached below. How to add it to this
page? Do I need to use git push, or just provide the patch? Could you
help put it in .SH VERSIONS? Thank you!
Best,
Zijun Zhao

On Fri, May 19, 2023 at 3:42 PM Alejandro Colomar
<alx.manpages@gmail.com> wrote:
>
> On 5/20/23 00:21, enh wrote:
> > On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar
> > <alx.manpages@gmail.com> wrote:
> [...]
>
> >> You'll also need to add _Nullable to the function prototypes in the
> >> SYNOPSIS.
> >
> > heh, funnily enough, zijunzhao's adding exactly those annotations to
> > the bionic headers, which is how we stumbled across this.
>
> :)
>
> >
> >> BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
> >> only settimeofday(3) that is nullable.
> >
> > yeah, but the _kernel_ allows null in both (see kernel/time/time.c),
> > and this is section 2 of the man page. it's unclear to me whether
> > that's worth calling out up here, or should be down in the
> > Linux-specific section? (this page seems to be written as if it cares
> > about Hurd? "On a non-Linux kernel, with glibc ...".)
>
> Yes, we care about hurd, and non-g libc, in this page and others.
> Normally, I would be cautious, and specify the strictest prototype of
> all implementations about which I care, but in this case, glibc seems
> unnecessarily restrictive, and I'm ready to add _Nullable if the kernel
> and some libc support that.  However, as a caution, please add a
> (CAVEATS?) section documenting that glibc doesn't allow NULL there.
> Or maybe keep the SYNOPSIS without _Nullable, and have some VERSIONS
> section specifying that other libcs and the kernel support NULL.
>
> >
> >> See:
> >>
> >>
> >> $ grepc gettimeofday /usr/include/*/sys/time.h
> >> /usr/include/x86_64-linux-gnu/sys/time.h:67:
> >> extern int gettimeofday (struct timeval *__restrict __tv,
> >>                          void *__restrict __tz) __THROW __nonnull ((1));
> >>
> >>
> >> /usr/include/x86_64-linux-gnu/sys/time.h:75:
> >> #  define gettimeofday __gettimeofday64
> >>
> >>
> >> $ grepc settimeofday /usr/include/*/sys/time.h
> >> /usr/include/x86_64-linux-gnu/sys/time.h:86:
> >> extern int settimeofday (const struct timeval *__tv,
> >>                          const struct timezone *__tz)
> >>       __THROW;
> >>
> >>
> >> /usr/include/x86_64-linux-gnu/sys/time.h:106:
> >> #   define settimeofday __settimeofday64
> >>
> >>
> >> And while NULL may be non-UB, the manual page is not very clear on why
> >> someone would want to call these functions with NULL.  Could you
> >> please also explain why would someone want to call these functions
> >> with NULL?  (Let's discuss it in the list, and then we see what
> >> wording we use for the page.)
> >
> > so there are definitely several orders of magnitude more users of a
> > null timezone --- most callers don't want that (not least because
> > glibc seems to not fill it out anyway? musl certainly doesn't,
> > implementing gettimeofday() in terms of clock_gettime() instead).
> >
> > the users for a null time (and non-null timezone; i haven't seen
> > anyone -- not even a test -- pass null for both!) are interesting.
> > Android's init uses settimeofday() with only a timezone, but --
> > although the timezone to use is settable in device-specific init
> > scripts -- for devices in our tree, that's actually always 0. there's
> > other proprietary (non-Google) code that uses gettimeofday() with only
> > a timezone, and i'm honestly not sure whether it's deliberate that
> > it's asking the _kernel_ for the timezone or not (i'm guessing they
> > want the device timezone rather than the app timezone, but i do not
> > believe there are devices where the _kernel_ timezone matches the
> > device timezone?). certainly both of these use cases seem worth
> > following up on, and i will do.
>
> Please.  And thanks for the details!
>
> >
> > but looking at musl (which ignores tz in both cases) it seems like the
> > most impactful change would be to fix "The functions gettimeofday()
> > and settimeofday() can get and set the time as well as a timezone" :-)
>
> :-)
>
> Makes sense; although it would be worth being clear about if that's
> really true, and if libcs do support that as well as the kernel.
>
> >
> > it also seems like an improvement to really call out the libc/kernel
> > differences more clearly though.
>
> Sure.  Many pages have a dedicated subsection.  Feel free to add it
> to this page.  I'd put it in .SH VERSIONS.
>
> $ grep -rn 'C library/kernel differences' man*
> man2/sigprocmask.2:120:.SS C library/kernel differences
> man2/setreuid.2:168:.SS C library/kernel differences
> man2/brk.2:126:.SS C library/kernel differences
> man2/sigwaitinfo.2:114:.SS C library/kernel differences
> man2/gethostname.2:122:.SS C library/kernel differences
> man2/getcpu.2:73:.SS C library/kernel differences
> man2/fork.2:263:.SS C library/kernel differences
> man2/epoll_wait.2:258:.SS C library/kernel differences
> man2/wait4.2:157:.SS C library/kernel differences
> man2/stat.2:355:.SS C library/kernel differences
> man2/sigsuspend.2:74:.SS C library/kernel differences
> man2/access.2:40:                /* But see C library/kernel differences, below */
> man2/access.2:285:.SS C library/kernel differences
> man2/clock_getres.2:354:.SS C library/kernel differences
> man2/getpid.2:57:.SS C library/kernel differences
> man2/setfsgid.2:57:.SS C library/kernel differences
> man2/getgroups.2:142:.SS C library/kernel differences
> man2/sigaction.2:946:.SS C library/kernel differences
> man2/setresuid.2:100:.SS C library/kernel differences
> man2/readv.2:290:.SS C library/kernel differences
> man2/readv.2:342:.SS Historical C library/kernel differences
> man2/open.2:1393:.SS C library/kernel differences
> man2/setgid.2:58:.SS C library/kernel differences
> man2/setfsuid.2:97:.SS C library/kernel differences
> man2/ptrace.2:2814:.SS C library/kernel differences
> man2/poll.2:382:.SS C library/kernel differences
> man2/sigreturn.2:130:.SS C library/kernel differences
> man2/eventfd.2:267:.SS C library/kernel differences
> man2/mmap.2:729:.SS C library/kernel differences
> man2/_exit.2:104:.SS C library/kernel differences
> man2/time.2:86:.SS C library/kernel differences
> man2/pread.2:101:.SS C library/kernel differences
> man2/seccomp.2:975:.IR "C library/kernel differences" .
> man2/setuid.2:104:.SS C library/kernel differences
> man2/sched_setaffinity.2:95:return 0 (but see "C library/kernel differences" below,
> man2/sched_setaffinity.2:220:.SS C library/kernel differences
> man2/select.2:559:.SS C library/kernel differences
> man2/sigpending.2:52:.SS C library/kernel differences
> man2/posix_fadvise.2:135:.SS C library/kernel differences
> man2/gettimeofday.2:173:.SS C library/kernel differences
> man2/uname.2:83:.SS C library/kernel differences
> man2/clone.2:1576:.SS C library/kernel differences
> man2/getpriority.2:185:.SS C library/kernel differences
> man2/signalfd.2:331:.SS C library/kernel differences
> man2/timer_create.2:225:.SS C library/kernel differences
> man2/wait.2:464:.SS C library/kernel differences
> man2/nice.2:80:.SS C library/kernel differences
> man2/chmod.2:303:.SS C library/kernel differences
> man2/seteuid.2:114:.SS C library/kernel differences
> man3/getcwd.3:217:.SS C library/kernel differences
> man3/killpg.3:98:.SS C library/kernel differences
> man3/mq_notify.3:174:.SS C library/kernel differences
> man3/mq_open.3:269:.SS C library/kernel differences
> man3/sigqueue.3:118:.SS C library/kernel differences
> man3type/epoll_event.3type:37:.SS C library/kernel differences
> man7/man-pages.7:402:.I "C library/kernel differences"
>
> Cheers,
> Alex
>
> >
> >> Thanks!
> >> Alex
> >>
> >> --
> >> <http://www.alejandro-colomar.es/>
> >> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
> >>
>
> --
> <http://www.alejandro-colomar.es/>
> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: 0001-Modify-the-documentation-of-gettimeofday.patch --]
[-- Type: application/octet-stream, Size: 1036 bytes --]

From 4df5bd711ec42de300cfe1c78589c0e1b48e73cb Mon Sep 17 00:00:00 2001
From: Zijun Zhao <zijunzhao@google.com>
Date: Mon, 5 Jun 2023 11:45:49 -0700
Subject: [PATCH] Modify the documentation of gettimeofday()

We find tv arg is allowed to be null in bionic so make the documentation more clear.
---
 man2/gettimeofday.2 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/man2/gettimeofday.2 b/man2/gettimeofday.2
index 9d134fa49..43f3fd9ff 100644
--- a/man2/gettimeofday.2
+++ b/man2/gettimeofday.2
@@ -175,6 +175,7 @@ On some architectures, an implementation of
 .BR gettimeofday ()
 is provided in the
 .BR vdso (7).
+The kernel accepts null for both time and timezone. The timezone argument is ignored by glibc and musl, and not passed to/from the kernel. Android's bionic passes the timezone argument to/from the kernel, but Android does not update the kernel timezone based on the device timezone in Settings, so the kernel's timezone is typically UTC.
 .SH STANDARDS
 .TP
 .BR gettimeofday ()
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH] Fix the man page

[Zijun Zhao]

Hi Alejandro,
  I already created the patch attached in the last email.How to add it
to this page? Do I need to use git push, or just provide the patch?
Could you
help put it in .SH VERSIONS? Thank you!
Best,
Zijun Zhao

On Mon, Jun 5, 2023 at 12:03 PM Zijun Zhao <zijunzhao@google.com> wrote:
>
> Hi Alejandro,
>   I already created the patch attached below. How to add it to this
> page? Do I need to use git push, or just provide the patch? Could you
> help put it in .SH VERSIONS? Thank you!
> Best,
> Zijun Zhao
>
> On Fri, May 19, 2023 at 3:42 PM Alejandro Colomar
> <alx.manpages@gmail.com> wrote:
> >
> > On 5/20/23 00:21, enh wrote:
> > > On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar
> > > <alx.manpages@gmail.com> wrote:
> > [...]
> >
> > >> You'll also need to add _Nullable to the function prototypes in the
> > >> SYNOPSIS.
> > >
> > > heh, funnily enough, zijunzhao's adding exactly those annotations to
> > > the bionic headers, which is how we stumbled across this.
> >
> > :)
> >
> > >
> > >> BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
> > >> only settimeofday(3) that is nullable.
> > >
> > > yeah, but the _kernel_ allows null in both (see kernel/time/time.c),
> > > and this is section 2 of the man page. it's unclear to me whether
> > > that's worth calling out up here, or should be down in the
> > > Linux-specific section? (this page seems to be written as if it cares
> > > about Hurd? "On a non-Linux kernel, with glibc ...".)
> >
> > Yes, we care about hurd, and non-g libc, in this page and others.
> > Normally, I would be cautious, and specify the strictest prototype of
> > all implementations about which I care, but in this case, glibc seems
> > unnecessarily restrictive, and I'm ready to add _Nullable if the kernel
> > and some libc support that.  However, as a caution, please add a
> > (CAVEATS?) section documenting that glibc doesn't allow NULL there.
> > Or maybe keep the SYNOPSIS without _Nullable, and have some VERSIONS
> > section specifying that other libcs and the kernel support NULL.
> >
> > >
> > >> See:
> > >>
> > >>
> > >> $ grepc gettimeofday /usr/include/*/sys/time.h
> > >> /usr/include/x86_64-linux-gnu/sys/time.h:67:
> > >> extern int gettimeofday (struct timeval *__restrict __tv,
> > >>                          void *__restrict __tz) __THROW __nonnull ((1));
> > >>
> > >>
> > >> /usr/include/x86_64-linux-gnu/sys/time.h:75:
> > >> #  define gettimeofday __gettimeofday64
> > >>
> > >>
> > >> $ grepc settimeofday /usr/include/*/sys/time.h
> > >> /usr/include/x86_64-linux-gnu/sys/time.h:86:
> > >> extern int settimeofday (const struct timeval *__tv,
> > >>                          const struct timezone *__tz)
> > >>       __THROW;
> > >>
> > >>
> > >> /usr/include/x86_64-linux-gnu/sys/time.h:106:
> > >> #   define settimeofday __settimeofday64
> > >>
> > >>
> > >> And while NULL may be non-UB, the manual page is not very clear on why
> > >> someone would want to call these functions with NULL.  Could you
> > >> please also explain why would someone want to call these functions
> > >> with NULL?  (Let's discuss it in the list, and then we see what
> > >> wording we use for the page.)
> > >
> > > so there are definitely several orders of magnitude more users of a
> > > null timezone --- most callers don't want that (not least because
> > > glibc seems to not fill it out anyway? musl certainly doesn't,
> > > implementing gettimeofday() in terms of clock_gettime() instead).
> > >
> > > the users for a null time (and non-null timezone; i haven't seen
> > > anyone -- not even a test -- pass null for both!) are interesting.
> > > Android's init uses settimeofday() with only a timezone, but --
> > > although the timezone to use is settable in device-specific init
> > > scripts -- for devices in our tree, that's actually always 0. there's
> > > other proprietary (non-Google) code that uses gettimeofday() with only
> > > a timezone, and i'm honestly not sure whether it's deliberate that
> > > it's asking the _kernel_ for the timezone or not (i'm guessing they
> > > want the device timezone rather than the app timezone, but i do not
> > > believe there are devices where the _kernel_ timezone matches the
> > > device timezone?). certainly both of these use cases seem worth
> > > following up on, and i will do.
> >
> > Please.  And thanks for the details!
> >
> > >
> > > but looking at musl (which ignores tz in both cases) it seems like the
> > > most impactful change would be to fix "The functions gettimeofday()
> > > and settimeofday() can get and set the time as well as a timezone" :-)
> >
> > :-)
> >
> > Makes sense; although it would be worth being clear about if that's
> > really true, and if libcs do support that as well as the kernel.
> >
> > >
> > > it also seems like an improvement to really call out the libc/kernel
> > > differences more clearly though.
> >
> > Sure.  Many pages have a dedicated subsection.  Feel free to add it
> > to this page.  I'd put it in .SH VERSIONS.
> >
> > $ grep -rn 'C library/kernel differences' man*
> > man2/sigprocmask.2:120:.SS C library/kernel differences
> > man2/setreuid.2:168:.SS C library/kernel differences
> > man2/brk.2:126:.SS C library/kernel differences
> > man2/sigwaitinfo.2:114:.SS C library/kernel differences
> > man2/gethostname.2:122:.SS C library/kernel differences
> > man2/getcpu.2:73:.SS C library/kernel differences
> > man2/fork.2:263:.SS C library/kernel differences
> > man2/epoll_wait.2:258:.SS C library/kernel differences
> > man2/wait4.2:157:.SS C library/kernel differences
> > man2/stat.2:355:.SS C library/kernel differences
> > man2/sigsuspend.2:74:.SS C library/kernel differences
> > man2/access.2:40:                /* But see C library/kernel differences, below */
> > man2/access.2:285:.SS C library/kernel differences
> > man2/clock_getres.2:354:.SS C library/kernel differences
> > man2/getpid.2:57:.SS C library/kernel differences
> > man2/setfsgid.2:57:.SS C library/kernel differences
> > man2/getgroups.2:142:.SS C library/kernel differences
> > man2/sigaction.2:946:.SS C library/kernel differences
> > man2/setresuid.2:100:.SS C library/kernel differences
> > man2/readv.2:290:.SS C library/kernel differences
> > man2/readv.2:342:.SS Historical C library/kernel differences
> > man2/open.2:1393:.SS C library/kernel differences
> > man2/setgid.2:58:.SS C library/kernel differences
> > man2/setfsuid.2:97:.SS C library/kernel differences
> > man2/ptrace.2:2814:.SS C library/kernel differences
> > man2/poll.2:382:.SS C library/kernel differences
> > man2/sigreturn.2:130:.SS C library/kernel differences
> > man2/eventfd.2:267:.SS C library/kernel differences
> > man2/mmap.2:729:.SS C library/kernel differences
> > man2/_exit.2:104:.SS C library/kernel differences
> > man2/time.2:86:.SS C library/kernel differences
> > man2/pread.2:101:.SS C library/kernel differences
> > man2/seccomp.2:975:.IR "C library/kernel differences" .
> > man2/setuid.2:104:.SS C library/kernel differences
> > man2/sched_setaffinity.2:95:return 0 (but see "C library/kernel differences" below,
> > man2/sched_setaffinity.2:220:.SS C library/kernel differences
> > man2/select.2:559:.SS C library/kernel differences
> > man2/sigpending.2:52:.SS C library/kernel differences
> > man2/posix_fadvise.2:135:.SS C library/kernel differences
> > man2/gettimeofday.2:173:.SS C library/kernel differences
> > man2/uname.2:83:.SS C library/kernel differences
> > man2/clone.2:1576:.SS C library/kernel differences
> > man2/getpriority.2:185:.SS C library/kernel differences
> > man2/signalfd.2:331:.SS C library/kernel differences
> > man2/timer_create.2:225:.SS C library/kernel differences
> > man2/wait.2:464:.SS C library/kernel differences
> > man2/nice.2:80:.SS C library/kernel differences
> > man2/chmod.2:303:.SS C library/kernel differences
> > man2/seteuid.2:114:.SS C library/kernel differences
> > man3/getcwd.3:217:.SS C library/kernel differences
> > man3/killpg.3:98:.SS C library/kernel differences
> > man3/mq_notify.3:174:.SS C library/kernel differences
> > man3/mq_open.3:269:.SS C library/kernel differences
> > man3/sigqueue.3:118:.SS C library/kernel differences
> > man3type/epoll_event.3type:37:.SS C library/kernel differences
> > man7/man-pages.7:402:.I "C library/kernel differences"
> >
> > Cheers,
> > Alex
> >
> > >
> > >> Thanks!
> > >> Alex
> > >>
> > >> --
> > >> <http://www.alejandro-colomar.es/>
> > >> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
> > >>
> >
> > --
> > <http://www.alejandro-colomar.es/>
> > GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

Re: [PATCH] Fix the man page

[enh]

(oh, and you'll have to switch to plain text mode or it'll bounce!)


On Fri, Jun 16, 2023 at 3:25 PM enh <enh@google.com> wrote:
>
> use `git format-patch -1` to generate a patch file, and then you can follow the instructions:
>
>        -  Send patches in diff -u format, inline inside the email message.  If
>           you're worried about your mailer breaking the patch, the send it
>           both inline and as an attachment.  You may find it useful to employ
>           git-send-email(1) and git-format-patch(1).
> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/CONTRIBUTING
>
> (note that gmail is very much a mailer that will break the patch, so you will need to send as an attachment.)
>
> On Fri, Jun 16, 2023 at 2:10 PM Zijun Zhao <zijunzhao@google.com> wrote:
>>
>> Hi Alejandro,
>>   I already created the patch attached in the last email.How to add it
>> to this page? Do I need to use git push, or just provide the patch?
>> Could you
>> help put it in .SH VERSIONS? Thank you!
>> Best,
>> Zijun Zhao
>>
>> On Mon, Jun 5, 2023 at 12:03 PM Zijun Zhao <zijunzhao@google.com> wrote:
>> >
>> > Hi Alejandro,
>> >   I already created the patch attached below. How to add it to this
>> > page? Do I need to use git push, or just provide the patch? Could you
>> > help put it in .SH VERSIONS? Thank you!
>> > Best,
>> > Zijun Zhao
>> >
>> > On Fri, May 19, 2023 at 3:42 PM Alejandro Colomar
>> > <alx.manpages@gmail.com> wrote:
>> > >
>> > > On 5/20/23 00:21, enh wrote:
>> > > > On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar
>> > > > <alx.manpages@gmail.com> wrote:
>> > > [...]
>> > >
>> > > >> You'll also need to add _Nullable to the function prototypes in the
>> > > >> SYNOPSIS.
>> > > >
>> > > > heh, funnily enough, zijunzhao's adding exactly those annotations to
>> > > > the bionic headers, which is how we stumbled across this.
>> > >
>> > > :)
>> > >
>> > > >
>> > > >> BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
>> > > >> only settimeofday(3) that is nullable.
>> > > >
>> > > > yeah, but the _kernel_ allows null in both (see kernel/time/time.c),
>> > > > and this is section 2 of the man page. it's unclear to me whether
>> > > > that's worth calling out up here, or should be down in the
>> > > > Linux-specific section? (this page seems to be written as if it cares
>> > > > about Hurd? "On a non-Linux kernel, with glibc ...".)
>> > >
>> > > Yes, we care about hurd, and non-g libc, in this page and others.
>> > > Normally, I would be cautious, and specify the strictest prototype of
>> > > all implementations about which I care, but in this case, glibc seems
>> > > unnecessarily restrictive, and I'm ready to add _Nullable if the kernel
>> > > and some libc support that.  However, as a caution, please add a
>> > > (CAVEATS?) section documenting that glibc doesn't allow NULL there.
>> > > Or maybe keep the SYNOPSIS without _Nullable, and have some VERSIONS
>> > > section specifying that other libcs and the kernel support NULL.
>> > >
>> > > >
>> > > >> See:
>> > > >>
>> > > >>
>> > > >> $ grepc gettimeofday /usr/include/*/sys/time.h
>> > > >> /usr/include/x86_64-linux-gnu/sys/time.h:67:
>> > > >> extern int gettimeofday (struct timeval *__restrict __tv,
>> > > >>                          void *__restrict __tz) __THROW __nonnull ((1));
>> > > >>
>> > > >>
>> > > >> /usr/include/x86_64-linux-gnu/sys/time.h:75:
>> > > >> #  define gettimeofday __gettimeofday64
>> > > >>
>> > > >>
>> > > >> $ grepc settimeofday /usr/include/*/sys/time.h
>> > > >> /usr/include/x86_64-linux-gnu/sys/time.h:86:
>> > > >> extern int settimeofday (const struct timeval *__tv,
>> > > >>                          const struct timezone *__tz)
>> > > >>       __THROW;
>> > > >>
>> > > >>
>> > > >> /usr/include/x86_64-linux-gnu/sys/time.h:106:
>> > > >> #   define settimeofday __settimeofday64
>> > > >>
>> > > >>
>> > > >> And while NULL may be non-UB, the manual page is not very clear on why
>> > > >> someone would want to call these functions with NULL.  Could you
>> > > >> please also explain why would someone want to call these functions
>> > > >> with NULL?  (Let's discuss it in the list, and then we see what
>> > > >> wording we use for the page.)
>> > > >
>> > > > so there are definitely several orders of magnitude more users of a
>> > > > null timezone --- most callers don't want that (not least because
>> > > > glibc seems to not fill it out anyway? musl certainly doesn't,
>> > > > implementing gettimeofday() in terms of clock_gettime() instead).
>> > > >
>> > > > the users for a null time (and non-null timezone; i haven't seen
>> > > > anyone -- not even a test -- pass null for both!) are interesting.
>> > > > Android's init uses settimeofday() with only a timezone, but --
>> > > > although the timezone to use is settable in device-specific init
>> > > > scripts -- for devices in our tree, that's actually always 0. there's
>> > > > other proprietary (non-Google) code that uses gettimeofday() with only
>> > > > a timezone, and i'm honestly not sure whether it's deliberate that
>> > > > it's asking the _kernel_ for the timezone or not (i'm guessing they
>> > > > want the device timezone rather than the app timezone, but i do not
>> > > > believe there are devices where the _kernel_ timezone matches the
>> > > > device timezone?). certainly both of these use cases seem worth
>> > > > following up on, and i will do.
>> > >
>> > > Please.  And thanks for the details!
>> > >
>> > > >
>> > > > but looking at musl (which ignores tz in both cases) it seems like the
>> > > > most impactful change would be to fix "The functions gettimeofday()
>> > > > and settimeofday() can get and set the time as well as a timezone" :-)
>> > >
>> > > :-)
>> > >
>> > > Makes sense; although it would be worth being clear about if that's
>> > > really true, and if libcs do support that as well as the kernel.
>> > >
>> > > >
>> > > > it also seems like an improvement to really call out the libc/kernel
>> > > > differences more clearly though.
>> > >
>> > > Sure.  Many pages have a dedicated subsection.  Feel free to add it
>> > > to this page.  I'd put it in .SH VERSIONS.
>> > >
>> > > $ grep -rn 'C library/kernel differences' man*
>> > > man2/sigprocmask.2:120:.SS C library/kernel differences
>> > > man2/setreuid.2:168:.SS C library/kernel differences
>> > > man2/brk.2:126:.SS C library/kernel differences
>> > > man2/sigwaitinfo.2:114:.SS C library/kernel differences
>> > > man2/gethostname.2:122:.SS C library/kernel differences
>> > > man2/getcpu.2:73:.SS C library/kernel differences
>> > > man2/fork.2:263:.SS C library/kernel differences
>> > > man2/epoll_wait.2:258:.SS C library/kernel differences
>> > > man2/wait4.2:157:.SS C library/kernel differences
>> > > man2/stat.2:355:.SS C library/kernel differences
>> > > man2/sigsuspend.2:74:.SS C library/kernel differences
>> > > man2/access.2:40:                /* But see C library/kernel differences, below */
>> > > man2/access.2:285:.SS C library/kernel differences
>> > > man2/clock_getres.2:354:.SS C library/kernel differences
>> > > man2/getpid.2:57:.SS C library/kernel differences
>> > > man2/setfsgid.2:57:.SS C library/kernel differences
>> > > man2/getgroups.2:142:.SS C library/kernel differences
>> > > man2/sigaction.2:946:.SS C library/kernel differences
>> > > man2/setresuid.2:100:.SS C library/kernel differences
>> > > man2/readv.2:290:.SS C library/kernel differences
>> > > man2/readv.2:342:.SS Historical C library/kernel differences
>> > > man2/open.2:1393:.SS C library/kernel differences
>> > > man2/setgid.2:58:.SS C library/kernel differences
>> > > man2/setfsuid.2:97:.SS C library/kernel differences
>> > > man2/ptrace.2:2814:.SS C library/kernel differences
>> > > man2/poll.2:382:.SS C library/kernel differences
>> > > man2/sigreturn.2:130:.SS C library/kernel differences
>> > > man2/eventfd.2:267:.SS C library/kernel differences
>> > > man2/mmap.2:729:.SS C library/kernel differences
>> > > man2/_exit.2:104:.SS C library/kernel differences
>> > > man2/time.2:86:.SS C library/kernel differences
>> > > man2/pread.2:101:.SS C library/kernel differences
>> > > man2/seccomp.2:975:.IR "C library/kernel differences" .
>> > > man2/setuid.2:104:.SS C library/kernel differences
>> > > man2/sched_setaffinity.2:95:return 0 (but see "C library/kernel differences" below,
>> > > man2/sched_setaffinity.2:220:.SS C library/kernel differences
>> > > man2/select.2:559:.SS C library/kernel differences
>> > > man2/sigpending.2:52:.SS C library/kernel differences
>> > > man2/posix_fadvise.2:135:.SS C library/kernel differences
>> > > man2/gettimeofday.2:173:.SS C library/kernel differences
>> > > man2/uname.2:83:.SS C library/kernel differences
>> > > man2/clone.2:1576:.SS C library/kernel differences
>> > > man2/getpriority.2:185:.SS C library/kernel differences
>> > > man2/signalfd.2:331:.SS C library/kernel differences
>> > > man2/timer_create.2:225:.SS C library/kernel differences
>> > > man2/wait.2:464:.SS C library/kernel differences
>> > > man2/nice.2:80:.SS C library/kernel differences
>> > > man2/chmod.2:303:.SS C library/kernel differences
>> > > man2/seteuid.2:114:.SS C library/kernel differences
>> > > man3/getcwd.3:217:.SS C library/kernel differences
>> > > man3/killpg.3:98:.SS C library/kernel differences
>> > > man3/mq_notify.3:174:.SS C library/kernel differences
>> > > man3/mq_open.3:269:.SS C library/kernel differences
>> > > man3/sigqueue.3:118:.SS C library/kernel differences
>> > > man3type/epoll_event.3type:37:.SS C library/kernel differences
>> > > man7/man-pages.7:402:.I "C library/kernel differences"
>> > >
>> > > Cheers,
>> > > Alex
>> > >
>> > > >
>> > > >> Thanks!
>> > > >> Alex
>> > > >>
>> > > >> --
>> > > >> <http://www.alejandro-colomar.es/>
>> > > >> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
>> > > >>
>> > >
>> > > --
>> > > <http://www.alejandro-colomar.es/>
>> > > GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

Re: [PATCH] Fix the man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 10480 bytes --]

Hi Zijun, enh!

On 6/17/23 00:26, enh wrote:
> (oh, and you'll have to switch to plain text mode or it'll bounce!)
> 
> 
> On Fri, Jun 16, 2023 at 3:25 PM enh <enh@google.com> wrote:
>>
>> use `git format-patch -1` to generate a patch file, and then you can follow the instructions:
>>
>>         -  Send patches in diff -u format, inline inside the email message.  If
>>            you're worried about your mailer breaking the patch, the send it
>>            both inline and as an attachment.  You may find it useful to employ
>>            git-send-email(1) and git-format-patch(1).
>> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/CONTRIBUTING
>>
>> (note that gmail is very much a mailer that will break the patch, so you will need to send as an attachment.)

Yup, enh's comments are good.  However, the reason that I didn't answer
for so long was just that I was on vacation.  :-)

I'll review your patch from the last one you sent to the list; no need
to resend.  For future patches, it would be good to do what enh said.
Please read <./CONTRIBUTING>.

Cheers,
Alex

>>
>> On Fri, Jun 16, 2023 at 2:10 PM Zijun Zhao <zijunzhao@google.com> wrote:
>>>
>>> Hi Alejandro,
>>>    I already created the patch attached in the last email.How to add it
>>> to this page? Do I need to use git push, or just provide the patch?
>>> Could you
>>> help put it in .SH VERSIONS? Thank you!
>>> Best,
>>> Zijun Zhao
>>>
>>> On Mon, Jun 5, 2023 at 12:03 PM Zijun Zhao <zijunzhao@google.com> wrote:
>>>>
>>>> Hi Alejandro,
>>>>    I already created the patch attached below. How to add it to this
>>>> page? Do I need to use git push, or just provide the patch? Could you
>>>> help put it in .SH VERSIONS? Thank you!
>>>> Best,
>>>> Zijun Zhao
>>>>
>>>> On Fri, May 19, 2023 at 3:42 PM Alejandro Colomar
>>>> <alx.manpages@gmail.com> wrote:
>>>>>
>>>>> On 5/20/23 00:21, enh wrote:
>>>>>> On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar
>>>>>> <alx.manpages@gmail.com> wrote:
>>>>> [...]
>>>>>
>>>>>>> You'll also need to add _Nullable to the function prototypes in the
>>>>>>> SYNOPSIS.
>>>>>>
>>>>>> heh, funnily enough, zijunzhao's adding exactly those annotations to
>>>>>> the bionic headers, which is how we stumbled across this.
>>>>>
>>>>> :)
>>>>>
>>>>>>
>>>>>>> BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
>>>>>>> only settimeofday(3) that is nullable.
>>>>>>
>>>>>> yeah, but the _kernel_ allows null in both (see kernel/time/time.c),
>>>>>> and this is section 2 of the man page. it's unclear to me whether
>>>>>> that's worth calling out up here, or should be down in the
>>>>>> Linux-specific section? (this page seems to be written as if it cares
>>>>>> about Hurd? "On a non-Linux kernel, with glibc ...".)
>>>>>
>>>>> Yes, we care about hurd, and non-g libc, in this page and others.
>>>>> Normally, I would be cautious, and specify the strictest prototype of
>>>>> all implementations about which I care, but in this case, glibc seems
>>>>> unnecessarily restrictive, and I'm ready to add _Nullable if the kernel
>>>>> and some libc support that.  However, as a caution, please add a
>>>>> (CAVEATS?) section documenting that glibc doesn't allow NULL there.
>>>>> Or maybe keep the SYNOPSIS without _Nullable, and have some VERSIONS
>>>>> section specifying that other libcs and the kernel support NULL.
>>>>>
>>>>>>
>>>>>>> See:
>>>>>>>
>>>>>>>
>>>>>>> $ grepc gettimeofday /usr/include/*/sys/time.h
>>>>>>> /usr/include/x86_64-linux-gnu/sys/time.h:67:
>>>>>>> extern int gettimeofday (struct timeval *__restrict __tv,
>>>>>>>                           void *__restrict __tz) __THROW __nonnull ((1));
>>>>>>>
>>>>>>>
>>>>>>> /usr/include/x86_64-linux-gnu/sys/time.h:75:
>>>>>>> #  define gettimeofday __gettimeofday64
>>>>>>>
>>>>>>>
>>>>>>> $ grepc settimeofday /usr/include/*/sys/time.h
>>>>>>> /usr/include/x86_64-linux-gnu/sys/time.h:86:
>>>>>>> extern int settimeofday (const struct timeval *__tv,
>>>>>>>                           const struct timezone *__tz)
>>>>>>>        __THROW;
>>>>>>>
>>>>>>>
>>>>>>> /usr/include/x86_64-linux-gnu/sys/time.h:106:
>>>>>>> #   define settimeofday __settimeofday64
>>>>>>>
>>>>>>>
>>>>>>> And while NULL may be non-UB, the manual page is not very clear on why
>>>>>>> someone would want to call these functions with NULL.  Could you
>>>>>>> please also explain why would someone want to call these functions
>>>>>>> with NULL?  (Let's discuss it in the list, and then we see what
>>>>>>> wording we use for the page.)
>>>>>>
>>>>>> so there are definitely several orders of magnitude more users of a
>>>>>> null timezone --- most callers don't want that (not least because
>>>>>> glibc seems to not fill it out anyway? musl certainly doesn't,
>>>>>> implementing gettimeofday() in terms of clock_gettime() instead).
>>>>>>
>>>>>> the users for a null time (and non-null timezone; i haven't seen
>>>>>> anyone -- not even a test -- pass null for both!) are interesting.
>>>>>> Android's init uses settimeofday() with only a timezone, but --
>>>>>> although the timezone to use is settable in device-specific init
>>>>>> scripts -- for devices in our tree, that's actually always 0. there's
>>>>>> other proprietary (non-Google) code that uses gettimeofday() with only
>>>>>> a timezone, and i'm honestly not sure whether it's deliberate that
>>>>>> it's asking the _kernel_ for the timezone or not (i'm guessing they
>>>>>> want the device timezone rather than the app timezone, but i do not
>>>>>> believe there are devices where the _kernel_ timezone matches the
>>>>>> device timezone?). certainly both of these use cases seem worth
>>>>>> following up on, and i will do.
>>>>>
>>>>> Please.  And thanks for the details!
>>>>>
>>>>>>
>>>>>> but looking at musl (which ignores tz in both cases) it seems like the
>>>>>> most impactful change would be to fix "The functions gettimeofday()
>>>>>> and settimeofday() can get and set the time as well as a timezone" :-)
>>>>>
>>>>> :-)
>>>>>
>>>>> Makes sense; although it would be worth being clear about if that's
>>>>> really true, and if libcs do support that as well as the kernel.
>>>>>
>>>>>>
>>>>>> it also seems like an improvement to really call out the libc/kernel
>>>>>> differences more clearly though.
>>>>>
>>>>> Sure.  Many pages have a dedicated subsection.  Feel free to add it
>>>>> to this page.  I'd put it in .SH VERSIONS.
>>>>>
>>>>> $ grep -rn 'C library/kernel differences' man*
>>>>> man2/sigprocmask.2:120:.SS C library/kernel differences
>>>>> man2/setreuid.2:168:.SS C library/kernel differences
>>>>> man2/brk.2:126:.SS C library/kernel differences
>>>>> man2/sigwaitinfo.2:114:.SS C library/kernel differences
>>>>> man2/gethostname.2:122:.SS C library/kernel differences
>>>>> man2/getcpu.2:73:.SS C library/kernel differences
>>>>> man2/fork.2:263:.SS C library/kernel differences
>>>>> man2/epoll_wait.2:258:.SS C library/kernel differences
>>>>> man2/wait4.2:157:.SS C library/kernel differences
>>>>> man2/stat.2:355:.SS C library/kernel differences
>>>>> man2/sigsuspend.2:74:.SS C library/kernel differences
>>>>> man2/access.2:40:                /* But see C library/kernel differences, below */
>>>>> man2/access.2:285:.SS C library/kernel differences
>>>>> man2/clock_getres.2:354:.SS C library/kernel differences
>>>>> man2/getpid.2:57:.SS C library/kernel differences
>>>>> man2/setfsgid.2:57:.SS C library/kernel differences
>>>>> man2/getgroups.2:142:.SS C library/kernel differences
>>>>> man2/sigaction.2:946:.SS C library/kernel differences
>>>>> man2/setresuid.2:100:.SS C library/kernel differences
>>>>> man2/readv.2:290:.SS C library/kernel differences
>>>>> man2/readv.2:342:.SS Historical C library/kernel differences
>>>>> man2/open.2:1393:.SS C library/kernel differences
>>>>> man2/setgid.2:58:.SS C library/kernel differences
>>>>> man2/setfsuid.2:97:.SS C library/kernel differences
>>>>> man2/ptrace.2:2814:.SS C library/kernel differences
>>>>> man2/poll.2:382:.SS C library/kernel differences
>>>>> man2/sigreturn.2:130:.SS C library/kernel differences
>>>>> man2/eventfd.2:267:.SS C library/kernel differences
>>>>> man2/mmap.2:729:.SS C library/kernel differences
>>>>> man2/_exit.2:104:.SS C library/kernel differences
>>>>> man2/time.2:86:.SS C library/kernel differences
>>>>> man2/pread.2:101:.SS C library/kernel differences
>>>>> man2/seccomp.2:975:.IR "C library/kernel differences" .
>>>>> man2/setuid.2:104:.SS C library/kernel differences
>>>>> man2/sched_setaffinity.2:95:return 0 (but see "C library/kernel differences" below,
>>>>> man2/sched_setaffinity.2:220:.SS C library/kernel differences
>>>>> man2/select.2:559:.SS C library/kernel differences
>>>>> man2/sigpending.2:52:.SS C library/kernel differences
>>>>> man2/posix_fadvise.2:135:.SS C library/kernel differences
>>>>> man2/gettimeofday.2:173:.SS C library/kernel differences
>>>>> man2/uname.2:83:.SS C library/kernel differences
>>>>> man2/clone.2:1576:.SS C library/kernel differences
>>>>> man2/getpriority.2:185:.SS C library/kernel differences
>>>>> man2/signalfd.2:331:.SS C library/kernel differences
>>>>> man2/timer_create.2:225:.SS C library/kernel differences
>>>>> man2/wait.2:464:.SS C library/kernel differences
>>>>> man2/nice.2:80:.SS C library/kernel differences
>>>>> man2/chmod.2:303:.SS C library/kernel differences
>>>>> man2/seteuid.2:114:.SS C library/kernel differences
>>>>> man3/getcwd.3:217:.SS C library/kernel differences
>>>>> man3/killpg.3:98:.SS C library/kernel differences
>>>>> man3/mq_notify.3:174:.SS C library/kernel differences
>>>>> man3/mq_open.3:269:.SS C library/kernel differences
>>>>> man3/sigqueue.3:118:.SS C library/kernel differences
>>>>> man3type/epoll_event.3type:37:.SS C library/kernel differences
>>>>> man7/man-pages.7:402:.I "C library/kernel differences"
>>>>>
>>>>> Cheers,
>>>>> Alex
>>>>>
>>>>>>
>>>>>>> Thanks!
>>>>>>> Alex
>>>>>>>
>>>>>>> --
>>>>>>> <http://www.alejandro-colomar.es/>
>>>>>>> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
>>>>>>>
>>>>>
>>>>> --
>>>>> <http://www.alejandro-colomar.es/>
>>>>> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix the man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2234 bytes --]

Hi Zijun,

Here goes some review of the patch.

> Subject: [PATCH] Modify the documentation of gettimeofday()

Please use 'gettimeofday.2:' as a prefix for the subject line,
and have a more descriptive subject (most patches do modify
the documentation, but how?).

See CONTRIBUTING:
        -  Write a suitable subject line.  Make sure to mention the
           name(s) of the page(s) being patched.  Example:

              [patch] shmop.2: Add "(void *)" cast to RETURN VALUE

> 
> We find tv arg is allowed to be null in bionic so make the documentation more clear.
> 
> Cc: enh <enh@google.com>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
>  man2/gettimeofday.2 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/man2/gettimeofday.2 b/man2/gettimeofday.2
> index 9d134fa49..43f3fd9ff 100644
> --- a/man2/gettimeofday.2
> +++ b/man2/gettimeofday.2
> @@ -175,6 +175,7 @@ .SS C library/kernel differences
>  .BR gettimeofday ()
>  is provided in the
>  .BR vdso (7).

A blank line should introduce the new paragraph, so add '.PP'.

> +The kernel accepts null for both time and timezone. The timezone argument is ignored by glibc and musl, and not passed to/from the kernel. Android's bionic passes the timezone argument to/from the kernel, but Android does not update the kernel timezone based on the device timezone in Settings, so the kernel's timezone is typically UTC.

Please respect the 80-column right margin.  If you use vim(1),
this may be useful to you:

     set colorcolumn=73,81

Also, please use semantic newlines.  See man-pages(7):

    Use semantic newlines
        In  the  source of a manual page, new sentences should be
        started on new lines, long sentences should be split into
        lines at clause breaks (commas, semicolons,  colons,  and
        so on), and long clauses should be split at phrase bound‐
        aries.   This  convention,  sometimes  known as "semantic
        newlines", makes it easier to see the effect of  patches,
        which often operate at the level of individual sentences,
        clauses, or phrases.

Cheers,
Alex

>  .SH STANDARDS
>  .TP
>  .BR gettimeofday ()
> -- 
> 2.40.1
> 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix the man page

[Zijun Zhao]

[-- Attachment #1: Type: text/plain, Size: 2845 bytes --]

Hi Alejandro,
  Sorry for the late reply! I made some changes(make the subject more
formal, add a blank line, respect the80-column right margin and use -u
to when doing git format-patch) and attached the patch below. But I am
a bit confused about semantic newlines. I think I already start on a
new line and use clause breaks to split long sentences? Do I
misunderstand something? Thank you!
Best,
Zijun Zhao


On Sat, Jul 8, 2023 at 11:37 AM Alejandro Colomar <alx@kernel.org> wrote:
>
> Hi Zijun,
>
> Here goes some review of the patch.
>
> > Subject: [PATCH] Modify the documentation of gettimeofday()
>
> Please use 'gettimeofday.2:' as a prefix for the subject line,
> and have a more descriptive subject (most patches do modify
> the documentation, but how?).
>
> See CONTRIBUTING:
>         -  Write a suitable subject line.  Make sure to mention the
>            name(s) of the page(s) being patched.  Example:
>
>               [patch] shmop.2: Add "(void *)" cast to RETURN VALUE
>
> >
> > We find tv arg is allowed to be null in bionic so make the documentation more clear.
> >
> > Cc: enh <enh@google.com>
> > Signed-off-by: Alejandro Colomar <alx@kernel.org>
> > ---
> >  man2/gettimeofday.2 | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/man2/gettimeofday.2 b/man2/gettimeofday.2
> > index 9d134fa49..43f3fd9ff 100644
> > --- a/man2/gettimeofday.2
> > +++ b/man2/gettimeofday.2
> > @@ -175,6 +175,7 @@ .SS C library/kernel differences
> >  .BR gettimeofday ()
> >  is provided in the
> >  .BR vdso (7).
>
> A blank line should introduce the new paragraph, so add '.PP'.
>
> > +The kernel accepts null for both time and timezone. The timezone argument is ignored by glibc and musl, and not passed to/from the kernel. Android's bionic passes the timezone argument to/from the kernel, but Android does not update the kernel timezone based on the device timezone in Settings, so the kernel's timezone is typically UTC.
>
> Please respect the 80-column right margin.  If you use vim(1),
> this may be useful to you:
>
>      set colorcolumn=73,81
>
> Also, please use semantic newlines.  See man-pages(7):
>
>     Use semantic newlines
>         In  the  source of a manual page, new sentences should be
>         started on new lines, long sentences should be split into
>         lines at clause breaks (commas, semicolons,  colons,  and
>         so on), and long clauses should be split at phrase bound‐
>         aries.   This  convention,  sometimes  known as "semantic
>         newlines", makes it easier to see the effect of  patches,
>         which often operate at the level of individual sentences,
>         clauses, or phrases.
>
> Cheers,
> Alex
>
> >  .SH STANDARDS
> >  .TP
> >  .BR gettimeofday ()
> > --
> > 2.40.1
> >

[-- Attachment #2: 0001-gettimeofday.2-Add-some-details-about-the-nullabilit.patch --]
[-- Type: application/octet-stream, Size: 1100 bytes --]

From d1592b56ca1239553d725be16b5bd1f87ed28161 Mon Sep 17 00:00:00 2001
From: Zijun Zhao <zijunzhao@google.com>
Date: Mon, 5 Jun 2023 11:45:49 -0700
Subject: [PATCH] gettimeofday.2: Add some details about the nullability of
 argument tv in different libraries.

We find tv arg is allowed to be null in bionic so make the documentation more clear.
---
 man2/gettimeofday.2 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/man2/gettimeofday.2 b/man2/gettimeofday.2
index 9d134fa49..a8309acf7 100644
--- a/man2/gettimeofday.2
+++ b/man2/gettimeofday.2
@@ -175,6 +175,12 @@ On some architectures, an implementation of
 .BR gettimeofday ()
 is provided in the
 .BR vdso (7).
+.PP
+The kernel accepts null for both time and timezone. The timezone
+argument is ignored by glibc and musl, and not passed to/from the
+kernel. Android's bionic passes the timezone argument to/from the
+kernel, but Android does not update the kernel timezone based on the
+device timezone in Settings, so the kernel's timezone is typically UTC.
 .SH STANDARDS
 .TP
 .BR gettimeofday ()
-- 
2.41.0.255.g8b1d071c50-goog


[-- Attachment #3: OpenPGP_signature --]
[-- Type: application/pgp-signature, Size: 849 bytes --]

Re: [PATCH] Fix the man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1828 bytes --]

Hi Zijun,

On 2023-07-12 01:48, Zijun Zhao wrote:
> Hi Alejandro,
>   Sorry for the late reply!

No problem :-)

> I made some changes(make the subject more
> formal, add a blank line, respect the80-column right margin and use -u
> to when doing git format-patch) and attached the patch below.

Thanks!

> But I am
> a bit confused about semantic newlines. I think I already start on a
> new line and use clause breaks to split long sentences? Do I
> misunderstand something? Thank you!

Here's your patch, modified to use semantic newlines:

+The kernel accepts null for both time and timezone.
+The timezone argument is ignored by glibc and musl,
+and not passed to/from the kernel.
+Android's bionic passes the timezone argument to/from the kernel,
+but Android does not update the kernel timezone
+based on the device timezone in Settings,
+so the kernel's timezone is typically UTC.

Does it make sense now?

I'll also comment a few things about the patch:

> +The kernel accepts null for both time and timezone.

We should use the 'tv' and 'tz' identifiers.  We should also use NULL.
I suggest:

The kernel accepts NULL for both
.I tv
and
.IR tz .

> +The timezone argument is ignored by glibc and musl,
> +and not passed to/from the kernel.
> +Android's bionic passes the timezone argument to/from the kernel,

Could you give an example of why bionic differs from glibc and musl,
and why it can be useful.  It is mostly curiosity, but it might be
useful to have it documented in the commit message.

> +but Android does not update the kernel timezone
> +based on the device timezone in Settings,
> +so the kernel's timezone is typically UTC.

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix the man page

[Zijun Zhao]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

Hi Alejandro,
  Sorry for the late reply! Thank you for your help! Yes, all comments
are very clear, understandable and helpful! This is the new patch.
Thank you for reviewing! Also, thank Elliott for helping! Hope this
patch is qualified!
Best,
Zijun


On Sat, Jul 15, 2023 at 9:13 AM Alejandro Colomar <alx@kernel.org> wrote:
>
> Hi Zijun,
>
> On 2023-07-12 01:48, Zijun Zhao wrote:
> > Hi Alejandro,
> >   Sorry for the late reply!
>
> No problem :-)
>
> > I made some changes(make the subject more
> > formal, add a blank line, respect the80-column right margin and use -u
> > to when doing git format-patch) and attached the patch below.
>
> Thanks!
>
> > But I am
> > a bit confused about semantic newlines. I think I already start on a
> > new line and use clause breaks to split long sentences? Do I
> > misunderstand something? Thank you!
>
> Here's your patch, modified to use semantic newlines:
>
> +The kernel accepts null for both time and timezone.
> +The timezone argument is ignored by glibc and musl,
> +and not passed to/from the kernel.
> +Android's bionic passes the timezone argument to/from the kernel,
> +but Android does not update the kernel timezone
> +based on the device timezone in Settings,
> +so the kernel's timezone is typically UTC.
>
> Does it make sense now?
>
> I'll also comment a few things about the patch:
>
> > +The kernel accepts null for both time and timezone.
>
> We should use the 'tv' and 'tz' identifiers.  We should also use NULL.
> I suggest:
>
> The kernel accepts NULL for both
> .I tv
> and
> .IR tz .
>
> > +The timezone argument is ignored by glibc and musl,
> > +and not passed to/from the kernel.
> > +Android's bionic passes the timezone argument to/from the kernel,
>
> Could you give an example of why bionic differs from glibc and musl,
> and why it can be useful.  It is mostly curiosity, but it might be
> useful to have it documented in the commit message.
>
> > +but Android does not update the kernel timezone
> > +based on the device timezone in Settings,
> > +so the kernel's timezone is typically UTC.
>
> Cheers,
> Alex
>
> --
> <http://www.alejandro-colomar.es/>
> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
>

[-- Attachment #2: 0001-gettimeofday.2-Add-some-details-about-the-nullabilit.patch --]
[-- Type: application/x-patch, Size: 1483 bytes --]

Re: [PATCH] Fix the man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2643 bytes --]

On 2023-07-27 20:33, Zijun Zhao wrote:
> Hi Alejandro,
>   Sorry for the late reply! Thank you for your help! Yes, all comments
> are very clear, understandable and helpful! This is the new patch.
> Thank you for reviewing! Also, thank Elliott for helping! Hope this
> patch is qualified!
> Best,
> Zijun

Heh, I replied to the wrong email.  Thanks for the patch; I applied it
(I applied this one, even if I replied to the other one).

Cheers,
Alex

> 
> 
> On Sat, Jul 15, 2023 at 9:13 AM Alejandro Colomar <alx@kernel.org> wrote:
>>
>> Hi Zijun,
>>
>> On 2023-07-12 01:48, Zijun Zhao wrote:
>>> Hi Alejandro,
>>>   Sorry for the late reply!
>>
>> No problem :-)
>>
>>> I made some changes(make the subject more
>>> formal, add a blank line, respect the80-column right margin and use -u
>>> to when doing git format-patch) and attached the patch below.
>>
>> Thanks!
>>
>>> But I am
>>> a bit confused about semantic newlines. I think I already start on a
>>> new line and use clause breaks to split long sentences? Do I
>>> misunderstand something? Thank you!
>>
>> Here's your patch, modified to use semantic newlines:
>>
>> +The kernel accepts null for both time and timezone.
>> +The timezone argument is ignored by glibc and musl,
>> +and not passed to/from the kernel.
>> +Android's bionic passes the timezone argument to/from the kernel,
>> +but Android does not update the kernel timezone
>> +based on the device timezone in Settings,
>> +so the kernel's timezone is typically UTC.
>>
>> Does it make sense now?
>>
>> I'll also comment a few things about the patch:
>>
>>> +The kernel accepts null for both time and timezone.
>>
>> We should use the 'tv' and 'tz' identifiers.  We should also use NULL.
>> I suggest:
>>
>> The kernel accepts NULL for both
>> .I tv
>> and
>> .IR tz .
>>
>>> +The timezone argument is ignored by glibc and musl,
>>> +and not passed to/from the kernel.
>>> +Android's bionic passes the timezone argument to/from the kernel,
>>
>> Could you give an example of why bionic differs from glibc and musl,
>> and why it can be useful.  It is mostly curiosity, but it might be
>> useful to have it documented in the commit message.
>>
>>> +but Android does not update the kernel timezone
>>> +based on the device timezone in Settings,
>>> +so the kernel's timezone is typically UTC.
>>
>> Cheers,
>> Alex
>>
>> --
>> <http://www.alejandro-colomar.es/>
>> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
>>

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix the man page

[Zijun Zhao]

Hi Alejandro,
  Excited to hear that! Thank you!
Best,
Zijun

On Fri, Jul 28, 2023 at 1:48 PM Alejandro Colomar <alx@kernel.org> wrote:
>
> On 2023-07-27 20:33, Zijun Zhao wrote:
> > Hi Alejandro,
> >   Sorry for the late reply! Thank you for your help! Yes, all comments
> > are very clear, understandable and helpful! This is the new patch.
> > Thank you for reviewing! Also, thank Elliott for helping! Hope this
> > patch is qualified!
> > Best,
> > Zijun
>
> Heh, I replied to the wrong email.  Thanks for the patch; I applied it
> (I applied this one, even if I replied to the other one).
>
> Cheers,
> Alex
>
> >
> >
> > On Sat, Jul 15, 2023 at 9:13 AM Alejandro Colomar <alx@kernel.org> wrote:
> >>
> >> Hi Zijun,
> >>
> >> On 2023-07-12 01:48, Zijun Zhao wrote:
> >>> Hi Alejandro,
> >>>   Sorry for the late reply!
> >>
> >> No problem :-)
> >>
> >>> I made some changes(make the subject more
> >>> formal, add a blank line, respect the80-column right margin and use -u
> >>> to when doing git format-patch) and attached the patch below.
> >>
> >> Thanks!
> >>
> >>> But I am
> >>> a bit confused about semantic newlines. I think I already start on a
> >>> new line and use clause breaks to split long sentences? Do I
> >>> misunderstand something? Thank you!
> >>
> >> Here's your patch, modified to use semantic newlines:
> >>
> >> +The kernel accepts null for both time and timezone.
> >> +The timezone argument is ignored by glibc and musl,
> >> +and not passed to/from the kernel.
> >> +Android's bionic passes the timezone argument to/from the kernel,
> >> +but Android does not update the kernel timezone
> >> +based on the device timezone in Settings,
> >> +so the kernel's timezone is typically UTC.
> >>
> >> Does it make sense now?
> >>
> >> I'll also comment a few things about the patch:
> >>
> >>> +The kernel accepts null for both time and timezone.
> >>
> >> We should use the 'tv' and 'tz' identifiers.  We should also use NULL.
> >> I suggest:
> >>
> >> The kernel accepts NULL for both
> >> .I tv
> >> and
> >> .IR tz .
> >>
> >>> +The timezone argument is ignored by glibc and musl,
> >>> +and not passed to/from the kernel.
> >>> +Android's bionic passes the timezone argument to/from the kernel,
> >>
> >> Could you give an example of why bionic differs from glibc and musl,
> >> and why it can be useful.  It is mostly curiosity, but it might be
> >> useful to have it documented in the commit message.
> >>
> >>> +but Android does not update the kernel timezone
> >>> +based on the device timezone in Settings,
> >>> +so the kernel's timezone is typically UTC.
> >>
> >> Cheers,
> >> Alex
> >>
> >> --
> >> <http://www.alejandro-colomar.es/>
> >> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
> >>
>
> --
> <http://www.alejandro-colomar.es/>
> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
>

Re: [PATCH] Fix the man page

[Zijun Zhao]

[-- Attachment #1: Type: text/plain, Size: 8148 bytes --]

Hi Alejandro,
  I already created the patch attached below. How to add it to this
page? Do I need to use git push, or just provide the patch? Could you
help put it in .SH VERSIONS? Thank you!
Best,
Zijun Zhao

On Fri, May 19, 2023 at 3:42 PM Alejandro Colomar
<alx.manpages@gmail.com> wrote:
>
> On 5/20/23 00:21, enh wrote:
> > On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar
> > <alx.manpages@gmail.com> wrote:
> [...]
>
> >> You'll also need to add _Nullable to the function prototypes in the
> >> SYNOPSIS.
> >
> > heh, funnily enough, zijunzhao's adding exactly those annotations to
> > the bionic headers, which is how we stumbled across this.
>
> :)
>
> >
> >> BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
> >> only settimeofday(3) that is nullable.
> >
> > yeah, but the _kernel_ allows null in both (see kernel/time/time.c),
> > and this is section 2 of the man page. it's unclear to me whether
> > that's worth calling out up here, or should be down in the
> > Linux-specific section? (this page seems to be written as if it cares
> > about Hurd? "On a non-Linux kernel, with glibc ...".)
>
> Yes, we care about hurd, and non-g libc, in this page and others.
> Normally, I would be cautious, and specify the strictest prototype of
> all implementations about which I care, but in this case, glibc seems
> unnecessarily restrictive, and I'm ready to add _Nullable if the kernel
> and some libc support that.  However, as a caution, please add a
> (CAVEATS?) section documenting that glibc doesn't allow NULL there.
> Or maybe keep the SYNOPSIS without _Nullable, and have some VERSIONS
> section specifying that other libcs and the kernel support NULL.
>
> >
> >> See:
> >>
> >>
> >> $ grepc gettimeofday /usr/include/*/sys/time.h
> >> /usr/include/x86_64-linux-gnu/sys/time.h:67:
> >> extern int gettimeofday (struct timeval *__restrict __tv,
> >>                          void *__restrict __tz) __THROW __nonnull ((1));
> >>
> >>
> >> /usr/include/x86_64-linux-gnu/sys/time.h:75:
> >> #  define gettimeofday __gettimeofday64
> >>
> >>
> >> $ grepc settimeofday /usr/include/*/sys/time.h
> >> /usr/include/x86_64-linux-gnu/sys/time.h:86:
> >> extern int settimeofday (const struct timeval *__tv,
> >>                          const struct timezone *__tz)
> >>       __THROW;
> >>
> >>
> >> /usr/include/x86_64-linux-gnu/sys/time.h:106:
> >> #   define settimeofday __settimeofday64
> >>
> >>
> >> And while NULL may be non-UB, the manual page is not very clear on why
> >> someone would want to call these functions with NULL.  Could you
> >> please also explain why would someone want to call these functions
> >> with NULL?  (Let's discuss it in the list, and then we see what
> >> wording we use for the page.)
> >
> > so there are definitely several orders of magnitude more users of a
> > null timezone --- most callers don't want that (not least because
> > glibc seems to not fill it out anyway? musl certainly doesn't,
> > implementing gettimeofday() in terms of clock_gettime() instead).
> >
> > the users for a null time (and non-null timezone; i haven't seen
> > anyone -- not even a test -- pass null for both!) are interesting.
> > Android's init uses settimeofday() with only a timezone, but --
> > although the timezone to use is settable in device-specific init
> > scripts -- for devices in our tree, that's actually always 0. there's
> > other proprietary (non-Google) code that uses gettimeofday() with only
> > a timezone, and i'm honestly not sure whether it's deliberate that
> > it's asking the _kernel_ for the timezone or not (i'm guessing they
> > want the device timezone rather than the app timezone, but i do not
> > believe there are devices where the _kernel_ timezone matches the
> > device timezone?). certainly both of these use cases seem worth
> > following up on, and i will do.
>
> Please.  And thanks for the details!
>
> >
> > but looking at musl (which ignores tz in both cases) it seems like the
> > most impactful change would be to fix "The functions gettimeofday()
> > and settimeofday() can get and set the time as well as a timezone" :-)
>
> :-)
>
> Makes sense; although it would be worth being clear about if that's
> really true, and if libcs do support that as well as the kernel.
>
> >
> > it also seems like an improvement to really call out the libc/kernel
> > differences more clearly though.
>
> Sure.  Many pages have a dedicated subsection.  Feel free to add it
> to this page.  I'd put it in .SH VERSIONS.
>
> $ grep -rn 'C library/kernel differences' man*
> man2/sigprocmask.2:120:.SS C library/kernel differences
> man2/setreuid.2:168:.SS C library/kernel differences
> man2/brk.2:126:.SS C library/kernel differences
> man2/sigwaitinfo.2:114:.SS C library/kernel differences
> man2/gethostname.2:122:.SS C library/kernel differences
> man2/getcpu.2:73:.SS C library/kernel differences
> man2/fork.2:263:.SS C library/kernel differences
> man2/epoll_wait.2:258:.SS C library/kernel differences
> man2/wait4.2:157:.SS C library/kernel differences
> man2/stat.2:355:.SS C library/kernel differences
> man2/sigsuspend.2:74:.SS C library/kernel differences
> man2/access.2:40:                /* But see C library/kernel differences, below */
> man2/access.2:285:.SS C library/kernel differences
> man2/clock_getres.2:354:.SS C library/kernel differences
> man2/getpid.2:57:.SS C library/kernel differences
> man2/setfsgid.2:57:.SS C library/kernel differences
> man2/getgroups.2:142:.SS C library/kernel differences
> man2/sigaction.2:946:.SS C library/kernel differences
> man2/setresuid.2:100:.SS C library/kernel differences
> man2/readv.2:290:.SS C library/kernel differences
> man2/readv.2:342:.SS Historical C library/kernel differences
> man2/open.2:1393:.SS C library/kernel differences
> man2/setgid.2:58:.SS C library/kernel differences
> man2/setfsuid.2:97:.SS C library/kernel differences
> man2/ptrace.2:2814:.SS C library/kernel differences
> man2/poll.2:382:.SS C library/kernel differences
> man2/sigreturn.2:130:.SS C library/kernel differences
> man2/eventfd.2:267:.SS C library/kernel differences
> man2/mmap.2:729:.SS C library/kernel differences
> man2/_exit.2:104:.SS C library/kernel differences
> man2/time.2:86:.SS C library/kernel differences
> man2/pread.2:101:.SS C library/kernel differences
> man2/seccomp.2:975:.IR "C library/kernel differences" .
> man2/setuid.2:104:.SS C library/kernel differences
> man2/sched_setaffinity.2:95:return 0 (but see "C library/kernel differences" below,
> man2/sched_setaffinity.2:220:.SS C library/kernel differences
> man2/select.2:559:.SS C library/kernel differences
> man2/sigpending.2:52:.SS C library/kernel differences
> man2/posix_fadvise.2:135:.SS C library/kernel differences
> man2/gettimeofday.2:173:.SS C library/kernel differences
> man2/uname.2:83:.SS C library/kernel differences
> man2/clone.2:1576:.SS C library/kernel differences
> man2/getpriority.2:185:.SS C library/kernel differences
> man2/signalfd.2:331:.SS C library/kernel differences
> man2/timer_create.2:225:.SS C library/kernel differences
> man2/wait.2:464:.SS C library/kernel differences
> man2/nice.2:80:.SS C library/kernel differences
> man2/chmod.2:303:.SS C library/kernel differences
> man2/seteuid.2:114:.SS C library/kernel differences
> man3/getcwd.3:217:.SS C library/kernel differences
> man3/killpg.3:98:.SS C library/kernel differences
> man3/mq_notify.3:174:.SS C library/kernel differences
> man3/mq_open.3:269:.SS C library/kernel differences
> man3/sigqueue.3:118:.SS C library/kernel differences
> man3type/epoll_event.3type:37:.SS C library/kernel differences
> man7/man-pages.7:402:.I "C library/kernel differences"
>
> Cheers,
> Alex
>
> >
> >> Thanks!
> >> Alex
> >>
> >> --
> >> <http://www.alejandro-colomar.es/>
> >> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
> >>
>
> --
> <http://www.alejandro-colomar.es/>
> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: 0001-Modify-the-documentation-of-gettimeofday.patch --]
[-- Type: application/x-patch, Size: 1036 bytes --]

Re: [PATCH] Fix the man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 8541 bytes --]

Hi Zijun,

On 2023-06-20 20:21, Zijun Zhao wrote:
> Hi Alejandro,
>   I already created the patch attached below. How to add it to this
> page? Do I need to use git push, or just provide the patch? Could you
> help put it in .SH VERSIONS? Thank you!
> Best,
> Zijun Zhao

I've applied the patch.

(You just need to provide the patch, as you did.  I apply and push it.)

Thanks!
Alex

> 
> On Fri, May 19, 2023 at 3:42 PM Alejandro Colomar
> <alx.manpages@gmail.com> wrote:
>>
>> On 5/20/23 00:21, enh wrote:
>>> On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar
>>> <alx.manpages@gmail.com> wrote:
>> [...]
>>
>>>> You'll also need to add _Nullable to the function prototypes in the
>>>> SYNOPSIS.
>>>
>>> heh, funnily enough, zijunzhao's adding exactly those annotations to
>>> the bionic headers, which is how we stumbled across this.
>>
>> :)
>>
>>>
>>>> BTW, I see that glibc still requires nonnull in gettimeofday(3).  It's
>>>> only settimeofday(3) that is nullable.
>>>
>>> yeah, but the _kernel_ allows null in both (see kernel/time/time.c),
>>> and this is section 2 of the man page. it's unclear to me whether
>>> that's worth calling out up here, or should be down in the
>>> Linux-specific section? (this page seems to be written as if it cares
>>> about Hurd? "On a non-Linux kernel, with glibc ...".)
>>
>> Yes, we care about hurd, and non-g libc, in this page and others.
>> Normally, I would be cautious, and specify the strictest prototype of
>> all implementations about which I care, but in this case, glibc seems
>> unnecessarily restrictive, and I'm ready to add _Nullable if the kernel
>> and some libc support that.  However, as a caution, please add a
>> (CAVEATS?) section documenting that glibc doesn't allow NULL there.
>> Or maybe keep the SYNOPSIS without _Nullable, and have some VERSIONS
>> section specifying that other libcs and the kernel support NULL.
>>
>>>
>>>> See:
>>>>
>>>>
>>>> $ grepc gettimeofday /usr/include/*/sys/time.h
>>>> /usr/include/x86_64-linux-gnu/sys/time.h:67:
>>>> extern int gettimeofday (struct timeval *__restrict __tv,
>>>>                          void *__restrict __tz) __THROW __nonnull ((1));
>>>>
>>>>
>>>> /usr/include/x86_64-linux-gnu/sys/time.h:75:
>>>> #  define gettimeofday __gettimeofday64
>>>>
>>>>
>>>> $ grepc settimeofday /usr/include/*/sys/time.h
>>>> /usr/include/x86_64-linux-gnu/sys/time.h:86:
>>>> extern int settimeofday (const struct timeval *__tv,
>>>>                          const struct timezone *__tz)
>>>>       __THROW;
>>>>
>>>>
>>>> /usr/include/x86_64-linux-gnu/sys/time.h:106:
>>>> #   define settimeofday __settimeofday64
>>>>
>>>>
>>>> And while NULL may be non-UB, the manual page is not very clear on why
>>>> someone would want to call these functions with NULL.  Could you
>>>> please also explain why would someone want to call these functions
>>>> with NULL?  (Let's discuss it in the list, and then we see what
>>>> wording we use for the page.)
>>>
>>> so there are definitely several orders of magnitude more users of a
>>> null timezone --- most callers don't want that (not least because
>>> glibc seems to not fill it out anyway? musl certainly doesn't,
>>> implementing gettimeofday() in terms of clock_gettime() instead).
>>>
>>> the users for a null time (and non-null timezone; i haven't seen
>>> anyone -- not even a test -- pass null for both!) are interesting.
>>> Android's init uses settimeofday() with only a timezone, but --
>>> although the timezone to use is settable in device-specific init
>>> scripts -- for devices in our tree, that's actually always 0. there's
>>> other proprietary (non-Google) code that uses gettimeofday() with only
>>> a timezone, and i'm honestly not sure whether it's deliberate that
>>> it's asking the _kernel_ for the timezone or not (i'm guessing they
>>> want the device timezone rather than the app timezone, but i do not
>>> believe there are devices where the _kernel_ timezone matches the
>>> device timezone?). certainly both of these use cases seem worth
>>> following up on, and i will do.
>>
>> Please.  And thanks for the details!
>>
>>>
>>> but looking at musl (which ignores tz in both cases) it seems like the
>>> most impactful change would be to fix "The functions gettimeofday()
>>> and settimeofday() can get and set the time as well as a timezone" :-)
>>
>> :-)
>>
>> Makes sense; although it would be worth being clear about if that's
>> really true, and if libcs do support that as well as the kernel.
>>
>>>
>>> it also seems like an improvement to really call out the libc/kernel
>>> differences more clearly though.
>>
>> Sure.  Many pages have a dedicated subsection.  Feel free to add it
>> to this page.  I'd put it in .SH VERSIONS.
>>
>> $ grep -rn 'C library/kernel differences' man*
>> man2/sigprocmask.2:120:.SS C library/kernel differences
>> man2/setreuid.2:168:.SS C library/kernel differences
>> man2/brk.2:126:.SS C library/kernel differences
>> man2/sigwaitinfo.2:114:.SS C library/kernel differences
>> man2/gethostname.2:122:.SS C library/kernel differences
>> man2/getcpu.2:73:.SS C library/kernel differences
>> man2/fork.2:263:.SS C library/kernel differences
>> man2/epoll_wait.2:258:.SS C library/kernel differences
>> man2/wait4.2:157:.SS C library/kernel differences
>> man2/stat.2:355:.SS C library/kernel differences
>> man2/sigsuspend.2:74:.SS C library/kernel differences
>> man2/access.2:40:                /* But see C library/kernel differences, below */
>> man2/access.2:285:.SS C library/kernel differences
>> man2/clock_getres.2:354:.SS C library/kernel differences
>> man2/getpid.2:57:.SS C library/kernel differences
>> man2/setfsgid.2:57:.SS C library/kernel differences
>> man2/getgroups.2:142:.SS C library/kernel differences
>> man2/sigaction.2:946:.SS C library/kernel differences
>> man2/setresuid.2:100:.SS C library/kernel differences
>> man2/readv.2:290:.SS C library/kernel differences
>> man2/readv.2:342:.SS Historical C library/kernel differences
>> man2/open.2:1393:.SS C library/kernel differences
>> man2/setgid.2:58:.SS C library/kernel differences
>> man2/setfsuid.2:97:.SS C library/kernel differences
>> man2/ptrace.2:2814:.SS C library/kernel differences
>> man2/poll.2:382:.SS C library/kernel differences
>> man2/sigreturn.2:130:.SS C library/kernel differences
>> man2/eventfd.2:267:.SS C library/kernel differences
>> man2/mmap.2:729:.SS C library/kernel differences
>> man2/_exit.2:104:.SS C library/kernel differences
>> man2/time.2:86:.SS C library/kernel differences
>> man2/pread.2:101:.SS C library/kernel differences
>> man2/seccomp.2:975:.IR "C library/kernel differences" .
>> man2/setuid.2:104:.SS C library/kernel differences
>> man2/sched_setaffinity.2:95:return 0 (but see "C library/kernel differences" below,
>> man2/sched_setaffinity.2:220:.SS C library/kernel differences
>> man2/select.2:559:.SS C library/kernel differences
>> man2/sigpending.2:52:.SS C library/kernel differences
>> man2/posix_fadvise.2:135:.SS C library/kernel differences
>> man2/gettimeofday.2:173:.SS C library/kernel differences
>> man2/uname.2:83:.SS C library/kernel differences
>> man2/clone.2:1576:.SS C library/kernel differences
>> man2/getpriority.2:185:.SS C library/kernel differences
>> man2/signalfd.2:331:.SS C library/kernel differences
>> man2/timer_create.2:225:.SS C library/kernel differences
>> man2/wait.2:464:.SS C library/kernel differences
>> man2/nice.2:80:.SS C library/kernel differences
>> man2/chmod.2:303:.SS C library/kernel differences
>> man2/seteuid.2:114:.SS C library/kernel differences
>> man3/getcwd.3:217:.SS C library/kernel differences
>> man3/killpg.3:98:.SS C library/kernel differences
>> man3/mq_notify.3:174:.SS C library/kernel differences
>> man3/mq_open.3:269:.SS C library/kernel differences
>> man3/sigqueue.3:118:.SS C library/kernel differences
>> man3type/epoll_event.3type:37:.SS C library/kernel differences
>> man7/man-pages.7:402:.I "C library/kernel differences"
>>
>> Cheers,
>> Alex
>>
>>>
>>>> Thanks!
>>>> Alex
>>>>
>>>> --
>>>> <http://www.alejandro-colomar.es/>
>>>> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
>>>>
>>
>> --
>> <http://www.alejandro-colomar.es/>
>> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]