Davide,
On Sun, 2007-03-11 at 16:04 -0700, Davide Libenzi wrote:
> +static int timerfd_setup(struct timerfd_ctx *ctx, int clockid, int tmrtype,
> + const struct itimerspec *ktmr)
> +{
> + enum hrtimer_mode htmode;
> + ktime_t texp, tintv;
> +
> + if (clockid != CLOCK_MONOTONIC &&
> + clockid != CLOCK_REALTIME)
> + return -EINVAL;
Please move the validation for clockid, tmrtype and the timerspec into
sys_timerfd. Do it before anything else. Also please validate both
it_value and it_interval unconditionally. Userspace should not send
uninitialized stuff at all.
The TFD_TIMER_SEQ thing is quite different to all other timer interfaces
which POSIX provides. Both itimers and posixtimers use the it_interval
value to distinguish between one shot and periodic timers.
I think we should keep this new interface analogous, so programmers
don't get more confused, than they are already. :)
This also allows relative and absolute starting points for both one shot
and sequential timers.
Please use it_value == 0 to stop the timer. This is the same as for
itimers and posixtimers. Right now you have to close the fd to stop a
timer, but that's not necessarily what you want.
Why do you want to store information, which is only relevant for setup
in ctx ?
If you do the validation right in sys_timerfd and get rid of
TFD_TIMER_SEQ and the various useless fields, then timerfd_setup() boils
down to
ctx->ticks = 0;
ctx->tintv = tintv;
hrtimer_init(&ctx->tmr, clockid, htmode);
ctx->tmr.function = timerfd_tmrproc;
if (texp.tv64 != 0)
hrtimer_start(&ctx->tmr, texp, htmode);
and in the timer function you simply check for
if (ctx->tintv.tv64 != 0)
instead of the TIMER_SEQ mode.
> +asmlinkage long sys_timerfd(int ufd, int clockid, int tmrtype,
> + const struct itimerspec __user *utmr)
> +{
> + int error;
> + struct timerfd_ctx *ctx;
> + struct file *file;
> + struct inode *inode;
> + struct itimerspec ktmr;
> +
> + if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
> + return -EFAULT;
Do validation of clockid, tmrtype and ktmr here.
> + if (ufd == -1) {
> + error = -ENOMEM;
> + ctx = kmem_cache_alloc(timerfd_ctx_cachep, GFP_KERNEL);
> + if (!ctx)
> + goto err_exit;
return -ENOMEM;
> + init_waitqueue_head(&ctx->wqh);
> + spin_lock_init(&ctx->lock);
> + ctx->clockid = -1;
> +
> + error = timerfd_setup(ctx, clockid, tmrtype, &ktmr);
> + if (error)
> + goto err_ctxfree;
> +
> + /*
> + * When we call this, the initialization must be complete, since
> + * aino_getfd() will install the fd.
> + */
> + error = aino_getfd(&ufd, &inode, &file, "[timerfd]",
> + &timerfd_fops, ctx);
> + if (error)
> + goto err_ctxfree;
> + } else {
> + error = -EBADF;
> + file = fget(ufd);
> + if (!file)
> + goto err_exit;
return -EBADF;
> + ctx = file->private_data;
> + error = -EINVAL;
> + if (file->f_op != &timerfd_fops) {
> + fput(file);
> + goto err_exit;
return -EINVAL;
> + }
> + /*
> + * We need to stop the exiting timer before.
> + */
-ENOPARSE. You probably mean: We need to stop an already running timer
before we do a new setup.
> + for (;;) {
> + spin_lock_irq(&ctx->lock);
> + if (hrtimer_try_to_cancel(&ctx->tmr) >= 0)
> + break;
> + spin_unlock_irq(&ctx->lock);
> + cpu_relax();
> + }
> + /*
> + * Re-program the timer to the new value ...
> + */
> + error = timerfd_setup(ctx, clockid, tmrtype, &ktmr);
> +
> + spin_unlock_irq(&ctx->lock);
> + fput(file);
> + if (error)
> + goto err_exit;
return error;
> + }
> +
> + return ufd;
> +
> +err_ctxfree:
> + timerfd_cleanup(ctx);
> +err_exit:
> + return error;
> +}
> +
tglx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
