Re: [Y2038] [PATCH 11/11] k_clock:Remove the 32bit methods with timespec type

On 20 April 2015 at 16:42, Richard Cochran richardcochran@gmail.com wrote:

On Mon, Apr 20, 2015 at 01:57:39PM +0800, Baolin Wang wrote:

...

@@ -911,18 +907,14 @@ retry: return -EINVAL;

  kc = clockid_to_kclock(timr->it_clock);

• if (WARN_ON_ONCE(!kc || (!kc->timer_set && !kc->timer_set64))) {
  

• if (WARN_ON_ONCE(!kc || !kc->timer_set64)) {
          error = -EINVAL;
  } else {
  

•         if (kc->timer_set64) {
  

•                 new_spec64 = itimerspec_to_itimerspec64(new_spec);
  

•                 error = kc->timer_set64(timr, flags, &new_spec64,
  

•                                         &old_spec64);
  

•                 if (old_setting)
  

•                         old_spec =
  

itimerspec64_to_itimerspec(old_spec64);

...

•         } else {
  

•                 error = kc->timer_set(timr, flags, &new_spec, rtn);
  

•         }
  

•         new_spec64 = itimerspec_to_itimerspec64(new_spec);
  

•         error = kc->timer_set64(timr, flags, &new_spec64,
  

•                                 &old_spec64);
  

This statement can fit on one line.

...

•         if (old_setting)
  

•                 old_spec = itimerspec64_to_itimerspec(old_spec64);
  }
  
  unlock_timer(timr, flag);
  

...

@@ -1057,14 +1045,13 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t,

which_clock,

...

  if (!kc)
          return -EINVAL;

• if (kc->clock_get64) {
  

•         error = kc->clock_get64(which_clock, &kernel_tp64);
  

•         kernel_tp = timespec64_to_timespec(kernel_tp64);
  

• } else {
  

•         error = kc->clock_get(which_clock, &kernel_tp);
  

• }
  

• error = kc->clock_get64(which_clock, &kernel_tp64);
  

• if (!error)
  

•         return error;
  

Wrong test, should be: if (error) ...

...

• kernel_tp = timespec64_to_timespec(kernel_tp64);
  
  

• if (!error && copy_to_user(tp, &kernel_tp, sizeof (kernel_tp)))
  

The (!error && ...) was correct here!

...

• if (copy_to_user(tp, &kernel_tp, sizeof (kernel_tp)))
          error = -EFAULT;
  
  return error;
  

You can simplify this like so:

    return copy_to_user(tp, &kernel_tp, sizeof(kernel_tp)) ? -EFAULT :

0;

...

@@ -1104,14 +1091,13 @@ SYSCALL_DEFINE2(clock_getres, const clockid_t,

which_clock,

...

  if (!kc)
          return -EINVAL;

• if (kc->clock_getres64) {
  

•         error = kc->clock_getres64(which_clock, &rtn_tp64);
  

•         rtn_tp = timespec64_to_timespec(rtn_tp64);
  

• } else {
  

•         error = kc->clock_getres(which_clock, &rtn_tp);
  

• }
  

• error = kc->clock_getres64(which_clock, &rtn_tp64);
  

• if (!error)
  

•         return error;
  

Also wrong.

...

• rtn_tp = timespec64_to_timespec(rtn_tp64);
  
  

• if (!error && tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp)))
  

• if (tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp)))
          error = -EFAULT;
  
  return error;
  

-- 1.7.9.5

Thanks, Richard

Thanks for your comments, i'll fix these mistakes in next patch series.