![]() | ||||||||||
Messages in this thread |
On Fri, 28 Mar 2008, Roland McGrath wrote:
>
> The control flow is less nonobvious without so much goto.
How about a further non-obviousness?
> +static int wait_consider_task(struct task_struct *parent,
> + struct task_struct *p, int *retval,
> + enum pid_type type, struct pid *pid, int options,
> + struct siginfo __user *infop,
> + int __user *stat_addr, struct rusage __user *ru)
> +{
...
> + if (task_is_stopped_or_traced(p)) {
> + /*
> + * It's stopped now, so it might later
> + * continue, exit, or stop again.
> + */
> + *retval = 0;
> + if ((p->ptrace & PT_PTRACED) ||
> + (options & WUNTRACED)) {
> + *retval = wait_task_stopped(p, (options & WNOWAIT),
> + infop, stat_addr, ru);
> + if (*retval)
> + return 1;
> + }
> + } else if (p->exit_state == EXIT_ZOMBIE && !delay_group_leader(p)) {
...
> + return 0;
I think it would be even more obvious (or, to use your phrase, "less
nonobvious") if this was written like so:
if (task_is_stopped_or_traced(p)) {
...
....
if (*retval}
return 1;
}
return 0;
}
if (p->exit_state == EXIT_ZOMBIE && !delay_group_leader(p)) {
...
return 0;
}
if (...)
because then you can clearly see that smething like the
"task_is_stopped_or_traced(p)" case is complete in itself, and only has
its own local stuff going on.
(And at some point I'd also almost make each case a trivial small inline
function of its on, but in this case there are so many arguments to pass
around that it probably becomes _less_ readable that way).
I also wonder if you really need both "int *retval" and the return value.
Isn't "retval" always going to be zero or a negative errno? And the return
value is going to be either true of false? Why not just fold them into one
single thing:
- negative: all done, with error
- zero: this didn't trigger, continue with the next one in caller
- positive: this thread triggered, all done, return 0 in the caller.
which is (I think) close to what we already do in eligible_child() (so
this would not be a new calling convention for this particular code).
Linus
--
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/
| |||||||||
| Last update: 2008-03-29 17:19 [from the cache] ©2003-2008 | ||||||||||