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

Richard Cochran
2015-04-20T08:42:30Z

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