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

[Baolin Wang <baolin.wang@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

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.



-- 
Baolin.wang
Best Regards

[-- Attachment #2: Type: text/html, Size: 4966 bytes --]