Messages in this thread | | | Date | Mon, 12 Mar 2007 11:46:27 -0700 (PDT) | From | Davide Libenzi <> | Subject | Re: [patch 6/9] signalfd/timerfd v3 - timerfd core ... |
| |
On Mon, 12 Mar 2007, Thomas Gleixner wrote:
> 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.
Ok.
> 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. :)
Yeah, this was already suggested by Nicholas Miell and was already in my code.
> 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.
This too.
> 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.
This was done too, although I did not know I could reference the tv64 member directly, so I had a macro checking the timespec.
> > + if (ufd == -1) { > > + error = -ENOMEM; > > + ctx = kmem_cache_alloc(timerfd_ctx_cachep, GFP_KERNEL); > > + if (!ctx) > > + goto err_exit; > > return -ENOMEM;
Ok.
> > + } > > + /* > > + * 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.
Bad description :)
- Davide
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |