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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx> wrote:On 5/20/23 00:21, enh wrote:On Fri, May 19, 2023 at 1:03 AM Alejandro Colomar <alx.manpages@xxxxxxxxx> 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, AlexThanks! 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:
OpenPGP_signature
Description: OpenPGP digital signature
