Re: [patch 05/14] syslets: core code

[Frederik Deweerdt <deweerdt@free.fr>]

On Thu, Feb 15, 2007 at 05:52:28PM +0100, Ingo Molnar wrote:
> From: Ingo Molnar <mingo@elte.hu>
> +static struct syslet_uatom __user *
> +exec_atom(struct async_head *ah, struct task_struct *t,
> +	  struct syslet_uatom __user *uatom)
> +{
> +	struct syslet_uatom __user *last_uatom;
> +	struct syslet_atom atom;
> +	long ret;
> +
- run_next:
+ 	do {
> +	if (unlikely(copy_uatom(&atom, uatom)))
> +		return ERR_PTR(-EFAULT);
> +
> +	last_uatom = uatom;
> +	ret = __exec_atom(t, &atom);
> +	if (unlikely(signal_pending(t) || need_resched()))
+ 		break;
- 		goto stop;
> +
> +	uatom = next_uatom(&atom, uatom, ret);
+ 	} while(uatom);
- 	if (uatom)
- 		goto run_next;
-  stop:
> +	/*
> +	 * We do completion only in async context:
> +	 */
> +	if (t->at && complete_uatom(ah, t, &atom, last_uatom))
> +		return ERR_PTR(-EFAULT);
> +
> +	return last_uatom;
> +}
Goto's are cool granted :), but IMO the "do {} while()" is more natural
here.
> +asmlinkage long
> +sys_async_register(struct async_head_user __user *ahu, unsigned int len)
> +{
> +	struct task_struct *t = current;
> +
> +	/*
> +	 * This 'len' check enables future extension of
> +	 * the async_head ABI:
> +	 */
I'm not sure I understand why this isn't a '>' check. If you want to
extend the ABI, you'll add members to async_head_user. So you just need
to check that you understand all the information that userspace passes
to you, or did I missed something?
> +	if (len != sizeof(struct async_head_user))
> +		return -EINVAL;
> +	/*
> +	 * Already registered?
> +	 */
> +	if (t->ah)
> +		return -EEXIST;
> +
> +	return async_head_init(t, ahu);
> +}
> +

Regards,
Frederik