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

[Richard Cochran]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/