This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Y2038: add struct __timespec64
- From: Joseph Myers <joseph at codesourcery dot com>
- To: "Albert ARIBAUD (3ADEV)" <albert dot aribaud at 3adev dot fr>
- Cc: <libc-alpha at sourceware dot org>
- Date: Wed, 19 Sep 2018 16:24:23 +0000
- Subject: Re: [PATCH] Y2038: add struct __timespec64
- References: <20180919072701.27535-1-albert.aribaud@3adev.fr>
On Wed, 19 Sep 2018, Albert ARIBAUD (3ADEV) wrote:
> To be Y2038-proof, struct __timespec64 needs its tv_sec field to
> be a __time64_t rather than a __time_t. However, the question is
> which type should the tv_nsec field be.
These comments are mainly about the proposed commit message, having
commented on the code changes separately. As before, the human-level part
should come *before* the ChangeLog entry to be maximally helpful to
reviewers.
I think this commit message is starting too deep into the details of the
patch. Before you delve into the details of the types of particular
fields of particular structures, you need to explain what exactly those
structures are for.
That is, your *first* paragraph needs to explain what struct __timespec64
is (an *internal* type like struct timespec, where the seconds field is
always 64-bit). Don't hide the information that this is a purely internal
type in the fourth paragraph as in the present commit message - it's
critical information that needs to be right up front. Once you've
explained the function of the type, then you can discuss constraints on
layout-compatibility with the public struct timespec when time_t is 64-bit
(whether that is the default, or whether that results from _TIME_BITS=64),
to avoid unnecessary copying at that interface boundary, and on
layout-compatibility with kernel interfaces, again to avoid unnecessary
copying. Only then should you get into the tv_nsec discussions - until
you've explained the function of and constraints on the struct
__timespec64 type, it makes no sense to get into the details of such
complications.
Having given the overall explanation of the layout requirements up front,
the discussion ot tv_nsec is reasonable enough - provided you are clear,
whenever you say things like
> Keeping tv_nsec a long (32-bit) would be compatible with Posix
about exactly what happens in the case where long is 64-bit and time_t is
already 64-bit. But there is another key piece of information missing
from this commit message: what are the kernel interfaces for 64-bit times
on 32-bit systems expected to look like?
We have, I think, made clear several times to the kernel people that the
best thing for glibc would be that, at the kernel/userspace ABI
boundaries, for ABIs where userspace long is 32-bit, only the low 32 bits
of the nanoseconds field should be considered significant when coming from
userspace to the kernel; the high bits should be considered as padding.
Will the kernel be implementing this?
If the kernel is implementing that, then there's no obvious need for this
structure with an explicit name for the padding at all, because there is
no need to copy 64-bit timespec values and zero the padding to pass them
into the kernel. On the other hand, if the kernel is not implementing
that - if the kernel is treating the nanoseconds field as a 64-bit
integer, all bits significant, when it comes from userspace, then copying
and zeroing padding is necessary when passing timespec values to the
kernel. So information about the kernel choice is critical information
for the glibc commit message to justify the particular choices made in
glibc.
Now if explicit zeroing is needed, the next question is whether the
internal type should have named padding, as here, or whether it should
have 64-bit nanoseconds. In the first case, the zeroing when passing
values to the kernel would be setting tv_pad to 0; in the second case, it
would just be a tv_nsec assignment and the compiler would deal with
setting the high part appropriately.
While the POSIX requirements are relevant for the *public* struct timespec
with _TIME_BITS=64, they are less clearly relevant for the internal type.
In either case, if pointers to one type are converted to pointers to the
other, an explicit type cast will be needed as the types won't be
compatible (except in the case where time_t is already 64-bit and long is
64-bit and __timespec64 can be #defined to timespec).
Finally, having explained the overall nature of the changes in the patch,
and then gone into the details of the particular choices of interest made
(such as for the type of tv_nsec), the human-level message for any patch
should then detail the testing done on that patch. This would include
testing (with the full glibc testsuite) for at least one 32-bit and one
64-bit platform (for such 64-bit time patches; appropriate testing is very
much dependent on the particular patch, and for many patches outside this
area there's no need to test on more than one platform). Then, after
that, the ChangeLog entry.
--
Joseph S. Myers
joseph@codesourcery.com