lkml.org 
[lkml]   [2006]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 1/2] kill_something_info: misc cleanups
Date
Christoph Hellwig <hch@infradead.org> writes:

> On Sat, Dec 16, 2006 at 11:05:10PM +0300, Oleg Nesterov wrote:
>> static int kill_something_info(int sig, struct siginfo *info, int pid)
>> {
>> int ret;
>> +
>> + rcu_read_lock();
>> + if (pid > 0) {
>> + ret = kill_pid_info(sig, info, find_pid(pid));
>> + } else if (pid == -1) {
>> + struct task_struct *p;
>> + int found = 0;
>> +
>> + ret = 0;
>> + read_lock(&tasklist_lock);
>> + for_each_process(p)
>> + if (!is_init(p) && p != current->group_leader) {
>> + int err = group_send_sig_info(sig, info, p);
>> + if (err != -EPERM)
>> + ret = err;
>> + found = 1;
>> + }
>> + read_unlock(&tasklist_lock);
>> + if (!found)
>> + ret = -ESRCH;
>
> This branch should probably be factored out into a helper of it's own:

The proper name would be something like kill_all_info(). As we are
talking about the group of all processes.

I am sitting here wondering why we bother to ignore init, as init
is protected from all signals it doesn't explicitly setup a signal
handler for. It is probably worth taking a quick look at the common
shutdown scripts and sysv init and see if anything actually cares if
we simply remove the is_init check.

The only two signals I know that are commonly handled this way
are kill(-1, SIGTERM) and kill(-1, SIGKILL);

And a very quick look at sysvinit-2.86 shows that it doesn't setup a
handler for SIGTERM. So I believe we can delete we can delete
the is_init check entirely without changing anything and with a less
surprising if anyone ever cares.

>> + } else {
>> + struct pid *grp = task_pgrp(current);
>> + if (pid != 0)
>> + grp = find_pid(-pid);
>> + ret = kill_pgrp_info(sig, info, grp);
>> + }
>
> This also looks rather unreadable, an
>
> } else if (pid) {
> ret = kill_pgrp_info(sig, info, find_pid(-pid));
> } else {
> ret = kill_pgrp_info(sig, info, task_pgrp(current));
> }
>
> might be slightly more code, but also a lot more readable.

And that part is basically what we have now, just reshuffled.

Eric
-
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: 2006-12-17 12:27    [W:0.147 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site