Re: [patch 10/13] signal/timer/event fds v8 - eventfd core ...

[Davide Libenzi]

On Fri, 30 Mar 2007, Andrew Morton wrote:

> > +struct eventfd_ctx {
> > +	spinlock_t lock;
> > +	wait_queue_head_t wqh;
> > +	__u64 count;
> > +};
> 
> Again, can we borrow wqh.lock?
> 
> `count' needs documentation - these things are key to understanding the
> code.

Added.



> > + */
> 
> So it is the caller's responsibility to ensure that *file refers to an
> eventfd file?

In which function? I lost you ...



> > +int eventfd_signal(struct file *file, int n)
> > +{
> > +	struct eventfd_ctx *ctx = file->private_data;
> > +	unsigned long flags;
> > +
> > +	if (n < 0)
> > +		return -EINVAL;
> > +	spin_lock_irqsave(&ctx->lock, flags);
> > +	if (ULLONG_MAX - ctx->count < n)
> > +		n = (int) (ULLONG_MAX - ctx->count);
> > +	ctx->count += n;
> > +	if (waitqueue_active(&ctx->wqh))
> > +		wake_up_locked(&ctx->wqh);
> > +	spin_unlock_irqrestore(&ctx->lock, flags);
> > +
> > +	return n;
> > +}
> 
> Neither the incoming arg (usefully named "n") nor the return value are
> documented.

Documented now.




> Needs interface documentation, please.  Even the changelog doesn't tell us
> what an EAGAIN return from read() means.

I'll be adding the errno documentation to all of them.




> > +static ssize_t eventfd_write(struct file *file, const char __user *buf, size_t count,
> > +			     loff_t *ppos)
> > +{
> > +	struct eventfd_ctx *ctx = file->private_data;
> > +	ssize_t res;
> > +	__u64 ucnt;
> > +	DECLARE_WAITQUEUE(wait, current);
> > +
> > +	if (count < sizeof(ucnt))
> > +		return -EINVAL;
> > +	if (get_user(ucnt, (const __u64 __user *) buf))
> > +		return -EFAULT;
> 
> Some architectures do not implement 64-bit get_user()

copy_from_user it is, then ...



- Davide


-
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/