On Thu, Feb 18, 2016 at 1:17 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Wednesday 17 February 2016 22:02:11 Deepa Dinamani wrote:
>>
>> I checked the server side code.
>> So the server side also reads whatever is sent over wire as u32 and
>> then assigns it to a time_t.
>
> I think the more important question is whether the server actually
> does anything with the timestamp other than pass it back to the
> client. If not, the interpretation it totally up to the client
> and the type on the server has no meaning (as long as it can store
> at least as many bits as the wire protocol, and passes all bits
> back the same way).
Timestamps do get interpreted and written to on the server according to the
files in the mds directory.
>> Do you have any documentation about the wire protocol?
>>
>> To me, the wire protocol seems to have been designed to support only
>> positive time values(u32).
>
> I think assigning to a time_t usually just means that this wasn't
> completely thought through when it got implemented (which we already
> know from the fact that it started out using a 32-bit number from
> the time). It's totally possible that this code originated on a 32-bit
> machine and was meant to encode a signed number and that the behavior
> changed without anyone noticing when it got ported to a 64-bit
> architecture for the first time.
>
>> This means ceph can represent times from 1970 - 2106 as long as both
>> server and client are 64 bit machines.
>> Or, client is 64 bit and server does not operate on time values.
>> And, on 32 bit machines implementation will be broken in 2038.
>
>> I will change back the above code to not use any casts(as they are a
>> no-op) as on the server side and note the above
>> things in the file and changelog:
>>
>> 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 = le32_to_cpu(tv->tv_sec);
>> + ts->tv_nsec = le32_to_cpu(tv->tv_nsec);
>> }
>
> Ok. I really like to have an explicit range-extension as well, but
> as you say, your patch above does not change behavior.
You mean change the functions and data types on both sides?
Not sure if this is meant for me or the Ceph team.
> I would write this as
>
> ts->tv_sec = (__u64)le32_to_cpu(tv->tv_sec);
> ts->tv_nsec = le32_to_cpu(tv->tv_nsec);
>
> which is again the same thing, but leaves less ambiguity.
Will do.
-Deepa
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
