Re: [PATCH] Fix the man page

[Alejandro Colomar <alx@kernel.org>]

[-- 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 --]