Actually I was thinking we'd do the opposite and document the existing 64-bit behavior, and then make sure we do it the same on 32-bit machines once we have the new syscalls.
I'm not sure I follow what is opposite here. You just want to document the behavior and fix it later when the time range is extended beyond 2038?
The most important part of course is to document what the file system is expected to do. Having this in the changelog is important here, but I'd also like to see a comment in the code to ensure readers can see that the behavior is intentional.
Will add this comment in the code.
diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h index a6ef9cc..e777e99 100644 --- a/include/linux/ceph/decode.h +++ b/include/linux/ceph/decode.h @@ -137,8 +137,8 @@ bad: static inline void ceph_decode_timespec(struct timespec *ts, const struct ceph_timespec *tv) {
ts->tv_sec = (__kernel_time_t)le32_to_cpu(tv->tv_sec);ts->tv_nsec = (long)le32_to_cpu(tv->tv_nsec);
ts->tv_sec = (s32)(s64)le32_to_cpu(tv->tv_sec);ts->tv_nsec = (s32)(s64)le32_to_cpu(tv->tv_nsec);}
Why did you choose to express this as "(s32)(s64)..." rather than "(s64)(s32)..."? The result is the same (just the s32 cast by itself would be sufficient), I just don't see yet how it clarifies what is going on.
Let's say we encode -1. so when you pass the same value to decode, ceph_timespec is {0xFFFFFFFF,0}. 0xFFFFFFFF is greater than INT_MAX. And, according to c99 the result is implementation dependent if this cast to a s32.
Quoting from http://c0x.coding-guidelines.com/6.3.1.3.html : 6.3.1.3 Signed and unsigned integers 1 When a value with integer type is converted to another integer type other than _Bool, if the value can be represented by the newtype, it is unchanged. 2 Otherwise, if the newtype is unsigned, the value is converted by repeatedly adding or subtracting one more than the maximum value that can be represented in the newtype until the value is in the range of the newtype.49) 3 Otherwise, the newtype is signed and the value cannot be represented in it; either the result is implementation-defined or an implementation-defined signal is raised
GCC does guarantee the behavior. Quoting from GCC manual (https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html): The result of, or the signal raised by, converting an integer to a signed integer type when the value cannot be represented in an object of that type (C90 6.2.1.2, C99 and C11 6.3.1.3). For conversion to a type of width N, the value is reduced modulo 2^N to be within range of the type; no signal is raised.
But, as the C standard suggests, behavior is implementation dependent. This is why I cast it to s64 before casting it to s32. I explained in the commit text, but missed GCC part. Quoting from commit text:
Consider the following scenario on a 32 bit system: When a negative number is cast to u32 as encode does, the value is positive and is greater than INT_MAX. Decode reads back this value. And, this value cannot be represented by long on 32 bit systems. So by section 6.3.1.3 of the C99 standard, the result is implementation defined.
Consider the following scenario on a 64 bit system: When a negative number is cast to u32 as encode does, the value is positive. This value is later assigned by decode function by a cast to long. Since this value can be represented in long data type, this becomes a positive value greater than INT_MAX. But, the value encoded was negative, so the encode and decode functions do not match.
Change the decode function as follows to overcome the above bug: The decode should first cast the value to a s64 this will be positive value greater than INT_MAX(in case of a negative encoded value)and then cast this value again as s32, which drops the higher order 32 bits. On 32 bit systems, this is the right value in kernel_time_t/ long. On 64 bit systems, assignment to kernel_time_t/ long will sign extend this value to reflect the signed bit encoded
-Deepa