Re: [PATCH] Y2038: add struct __timespec64

[Joseph Myers <joseph at codesourcery dot com>]

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