Re: [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

[Thomas Gleixner]

On Sat, 18 Mar 2017, Deepa Dinamani wrote:

>  struct timespec is not y2038 safe.
>  Replace the posix_clock ops interfaces to use
>  struct timespec64.
>  The patch also changes struct itimerspec interfaces to
>  struct itimerspec64 as itimerspec internally uses timespec
>  and itimerspec64 uses timespec64.
>  PTP clocks is the only module that sets up these interfaces.
>  All individual drivers rely on PTP class driver for exposure
>  to userspace. Hence, the change also deals with fixing up these
>  PTP interfaces.
>  The patch also changes dynamic posix clock implementation to
>  reflect the changes in the functional clock interface.

Please do not explain WHAT the patch is doing. We can see that from the
diff itself. What's important is the WHY. A good changelog is structured in
paragraphs, which explain the context, the problem and the solution. Please
read Documentation/process/submitting-patches.rst. Let me give you an
example.

  struct timespec is not Y2038 safe on 32 bit machines and needs to be
  replaced with struct timespec64.

  The posix clock functions use struct timespec directly and through struct
  itimerspec.

  Change all function prototypes to use timespec64 and itimerspec64 and fix
  up all implementations.

That gives all the information a reviewer or someone who is looking at the
commit later needs: context, problem scope and solution.

Hmm?

>  /* posix clock implementation */
>  
> -static int ptp_clock_getres(struct posix_clock *pc, struct timespec *tp)
> +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 *tp)

That's a pretty pointless exercise. getres() returns the resolution of the
clock which obviously can never be affected by Y2038. 

>  static int pc_clock_settime(clockid_t id, const struct timespec *ts)
>  {
>  	struct posix_clock_desc cd;
> +	struct timespec64 ts64 = timespec_to_timespec64(*ts);
>  	int err;

Please order the variables as a reverse fir tree sorted by length.

	struct timespec64 ts64 = timespec_to_timespec64(*ts);
 	struct posix_clock_desc cd;
 	int err;

That's way simpler to parse than the above random length odering.

> @@ -418,14 +427,19 @@ static int pc_timer_settime(struct k_itimer *kit, int flags,
>  {
>  	clockid_t id = kit->it_clock;
>  	struct posix_clock_desc cd;
> +	struct itimerspec64 old64;
> +	struct itimerspec64 ts64 = itimerspec_to_itimerspec64(ts);

See above.

Thanks,

	tglx