Messages in this thread | | | Date | Tue, 16 Dec 2008 11:16:21 +0100 | From | Oleg Nesterov <> | Subject | Re: [RFC][PATCH] Fix si_pid in send_signal() for SEND_SIG_NOINFO |
| |
On 12/15, Sukadev Bhattiprolu wrote: > > Oleg Nesterov [oleg@redhat.com] wrote: > | On 12/12, Sukadev Bhattiprolu wrote: > | > > | > --- a/kernel/signal.c > | > +++ b/kernel/signal.c > | > @@ -856,7 +856,8 @@ static int send_signal(int sig, struct siginfo *info, struct task_struct *t, > | > q->info.si_signo = sig; > | > q->info.si_errno = 0; > | > q->info.si_code = SI_USER; > | > - q->info.si_pid = task_pid_vnr(current); > | > + q->info.si_pid = task_tgid_nr_ns(current, > | > + task_active_pid_ns(t)); > | > | I agree with the patch, but you also changed "pid" to "tgid", > | and the changelog says nothing about this. > > Yes, I should have mentioned it in the changelog. > | > | Actually, this looks more correct to me, but did you check we > | don't break something? > > Besides running some tests and LTP and posting as [rfc] not sure how > else to check for breakage :-) Appreciate any pointers to tests for > signals. > > I had seen an earlier version of this patch which mentioned that > POSIX requires tgid rather than pid. And that looked logical to > me
Let me repeat, I think this is more correct too, and personally I agree with this change.
But I do not know if this change is "safe", please see below.
> and did not know about follow requirement on pdeath_signal.
Sorry for confusion, I didn't mean this will confuse the users of ->pdeath_signal, I just do not know. But they need the notificatation about the thread, not process.
> | For example, ->pdeath_signal. Note that we send it even if we > | re-parent to sub-thread, not to our init. Yes, yes, yes. This > | is imho very ugly and we should stop doing this. But we have > | users which claim they need exactly this behaviour. > > Ok. so should I change it to task_pid_nr_ns() ?
I don't know. I vote for your patch as is. (but please update the changelog).
> Would that still > be correct for say the SIGPIPE from pipe_write() ?
Again, I don't know.
But. Let's suppose we have the user-space application which uses .si_pid to figure out which thread hits SIGPIPE. Yes, this is ugly because the signal is thread-specific, but this works until this patch.
Actually, I don't understand most of (all?) users of send_signal(SIGXXX, current, 0), SEND_SIG_PRIV looks more logical to me. Or, _perhaps_, we need yet another SEND_SIG_SELF which sets .si_pid = task_pid_vnr(target).
Oleg.
| |