On Thu, Feb 18, 2016 at 10:35 AM, Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
> 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.
If we are going to touch this function at all, let's drop all casts and
just add a comment along the lines of "the lack of cast/sign-extension
here is intentional, this function has a counterpart on the server side
which also lacks cast/sign-extension, etc". To someone who is not up
on these subtleties, the __u64 cast is going to be more confusing than
explicit...
Thanks,
Ilya
--
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
