On Thu, Feb 19, 2004 at 10:42:56AM +0800, Isaac To wrote:
> >>>>> "Andrew" == Andrew Suffield <asuffield@debian.org> writes:
>
> Andrew> It does if you do it right. I don't exactly write this sort of
> Andrew> code infrequently; it's quite easy to structure your code in a
> Andrew> manner that works neatly and effectively.
>
> Perhaps you want to enlighten the kernel developers.
Not particularly. I am very unlikely to ever work on that code.
> A typical kernel
> function, say sys_getpgid, is implemented (in 2.4.24 kernel) like this:
>
> asmlinkage long sys_setpgid(pid_t pid, pid_t pgid)
> {
> struct task_struct * p;
> int err = -EINVAL;
>
> if (!pid)
> pid = current->pid;
> if (!pgid)
> pgid = pid;
> if (pgid < 0)
> return -EINVAL;
>
> /* From this point forward we keep holding onto the tasklist lock
> * so that our parent does not change from under us. -DaveM
> */
> read_lock(&tasklist_lock);
>
> err = -ESRCH;
> p = find_task_by_pid(pid);
> if (!p)
> goto out;
>
> if (p->p_pptr == current || p->p_opptr == current) {
> err = -EPERM;
> if (p->session != current->session)
> goto out;
> err = -EACCES;
> if (p->did_exec)
> goto out;
> } else if (p != current)
> goto out;
> err = -EPERM;
> if (p->leader)
> goto out;
> if (pgid != pid) {
> struct task_struct * tmp;
> for_each_task (tmp) {
> if (tmp->pgrp == pgid &&
> tmp->session == current->session)
> goto ok_pgid;
> }
> goto out;
> }
>
> ok_pgid:
> p->pgrp = pgid;
> err = 0;
> out:
> /* All paths lead to here, thus we are safe. -DaveM */
> read_unlock(&tasklist_lock);
> return err;
> }
And here's how I'd do it (just fixing this one issue; I wouldn't have
written the rest like this anyway - I'd have used some comments, for
starters):
static long
do_setpgid(pid_t pid, pid_t pgid)
{
struct task_struct * p;
p = find_task_by_pid(pid);
if (!p)
return -ESRCH;
if (p->p_pptr == current || p->p_opptr == current) {
if (p->session != current->session)
return -EPERM;
if (p->did_exec)
return -EACCES;
} else if (p != current)
return -ESRCH;
if (p->leader)
return -EPERM;
if (pgid != pid) {
struct task_struct * tmp;
for_each_task (tmp) {
if (tmp->pgrp == pgid &&
tmp->session == current->session)
{
p->pgrp = pgid;
return 0;
}
}
return -EPERM;
}
}
asmlinkage long sys_setpgid(pid_t pid, pid_t pgid)
{
long ret;
if (!pid)
pid = current->pid;
if (!pgid)
pgid = pid;
if (pgid < 0)
return -EINVAL;
read_lock(&tasklist_lock);
ret = do_setpgid(pid, pgid);
read_unlock(&tasklist_lock);
return ret;
}
(The idiom here is: check input, acquire resources, process, release
resources, and split those out into other functions as needed - strict
two-phase locking, which avoids deadlocks into the bargain)
No gotos, no need to worry about the current value of err, and
significant code is cleanly separated from locking and argument
checking. And both functions are rather easier to read (if a function
is too big to fit entirely on screen at once, you probably need to
split it up - the exception is when it's caused by huge comments).
Most problems in computer science are solved by adding another layer
of abstraction.
> For me, if "goto" is used in a disciplined way, one shouldn't be afraid to
> use them, especially if it simplify your code.
It never simplifies code. That's been the whole point for the past
Eris-knows-how-many years. Any time that you think you've found a use
of goto that makes the code simpler, there's another way to write it
without goto that would be simpler still. Example above.
--
.''`. ** Debian GNU/Linux ** | Andrew Suffield
: :' : http://www.debian.org/ |
`. `' |
`- -><- |
Attachment:
signature.asc
Description: Digital signature