Leesoo Ahn <d...@ooseel.net> writes: > clear_siginfo() is responsible for clearing struct kernel_siginfo object. > It's obvious that manually initializing those fields is needless as > a commit[1] explains why the function introduced and its guarantee that > all bits in the struct are cleared after it.
The fields that are explicitly set to zero are fields that must be zero. Not fields whose value should default to zero. Given how difficult it is to keep track of which fields are relevant in struct siginfo. I prefer the explicit approach that currently exists. Especially as the compiler appears smart enough to optimize through this. Mostly I am being touchy and conservative because getting all of these fields set properly took me several kernel development cycles, and before that there were bugs in setting those fields that persisted for decades. I think the current form allows someone to glance at the entry and see that a field like si_errno is present and set to 0. With you your suggested change someone needs to walk through the definition of the union and see which union member is being invoked to see which fields are present, to see if si_errno even exists, before it is possible to see that si_errno is 0. So unless there is a reason more compelling than a few less lines of code let's keep it this way for now. Eric > [1]: commit 8c5dbf2ae00b ("signal: Introduce clear_siginfo") > > Signed-off-by: Leesoo Ahn <ls...@ooseel.net> > --- > kernel/signal.c | 21 --------------------- > 1 file changed, 21 deletions(-) > > diff --git a/kernel/signal.c b/kernel/signal.c > index 5736c55aaa1a..8f49fa3ade33 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -603,10 +603,7 @@ static void collect_signal(int sig, struct sigpending > *list, kernel_siginfo_t *i > */ > clear_siginfo(info); > info->si_signo = sig; > - info->si_errno = 0; > info->si_code = SI_USER; > - info->si_pid = 0; > - info->si_uid = 0; > } > } > > @@ -1120,7 +1117,6 @@ static int __send_signal(int sig, struct kernel_siginfo > *info, struct task_struc > case (unsigned long) SEND_SIG_NOINFO: > clear_siginfo(&q->info); > q->info.si_signo = sig; > - q->info.si_errno = 0; > q->info.si_code = SI_USER; > q->info.si_pid = task_tgid_nr_ns(current, > task_active_pid_ns(t)); > @@ -1133,10 +1129,7 @@ static int __send_signal(int sig, struct > kernel_siginfo *info, struct task_struc > case (unsigned long) SEND_SIG_PRIV: > clear_siginfo(&q->info); > q->info.si_signo = sig; > - q->info.si_errno = 0; > q->info.si_code = SI_KERNEL; > - q->info.si_pid = 0; > - q->info.si_uid = 0; > break; > default: > copy_siginfo(&q->info, info); > @@ -1623,10 +1616,7 @@ void force_sig(int sig) > > clear_siginfo(&info); > info.si_signo = sig; > - info.si_errno = 0; > info.si_code = SI_KERNEL; > - info.si_pid = 0; > - info.si_uid = 0; > force_sig_info(&info); > } > EXPORT_SYMBOL(force_sig); > @@ -1659,7 +1649,6 @@ int force_sig_fault_to_task(int sig, int code, void > __user *addr > > clear_siginfo(&info); > info.si_signo = sig; > - info.si_errno = 0; > info.si_code = code; > info.si_addr = addr; > #ifdef __ARCH_SI_TRAPNO > @@ -1691,7 +1680,6 @@ int send_sig_fault(int sig, int code, void __user *addr > > clear_siginfo(&info); > info.si_signo = sig; > - info.si_errno = 0; > info.si_code = code; > info.si_addr = addr; > #ifdef __ARCH_SI_TRAPNO > @@ -1712,7 +1700,6 @@ int force_sig_mceerr(int code, void __user *addr, short > lsb) > WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR)); > clear_siginfo(&info); > info.si_signo = SIGBUS; > - info.si_errno = 0; > info.si_code = code; > info.si_addr = addr; > info.si_addr_lsb = lsb; > @@ -1726,7 +1713,6 @@ int send_sig_mceerr(int code, void __user *addr, short > lsb, struct task_struct * > WARN_ON((code != BUS_MCEERR_AO) && (code != BUS_MCEERR_AR)); > clear_siginfo(&info); > info.si_signo = SIGBUS; > - info.si_errno = 0; > info.si_code = code; > info.si_addr = addr; > info.si_addr_lsb = lsb; > @@ -1740,7 +1726,6 @@ int force_sig_bnderr(void __user *addr, void __user > *lower, void __user *upper) > > clear_siginfo(&info); > info.si_signo = SIGSEGV; > - info.si_errno = 0; > info.si_code = SEGV_BNDERR; > info.si_addr = addr; > info.si_lower = lower; > @@ -1755,7 +1740,6 @@ int force_sig_pkuerr(void __user *addr, u32 pkey) > > clear_siginfo(&info); > info.si_signo = SIGSEGV; > - info.si_errno = 0; > info.si_code = SEGV_PKUERR; > info.si_addr = addr; > info.si_pkey = pkey; > @@ -1934,7 +1918,6 @@ bool do_notify_parent(struct task_struct *tsk, int sig) > > clear_siginfo(&info); > info.si_signo = sig; > - info.si_errno = 0; > /* > * We are under tasklist_lock here so our parent is tied to > * us and cannot change. > @@ -2033,7 +2016,6 @@ static void do_notify_parent_cldstop(struct task_struct > *tsk, > > clear_siginfo(&info); > info.si_signo = SIGCHLD; > - info.si_errno = 0; > /* > * see comment in do_notify_parent() about the following 4 lines > */ > @@ -2506,7 +2488,6 @@ static int ptrace_signal(int signr, kernel_siginfo_t > *info) > if (signr != info->si_signo) { > clear_siginfo(info); > info->si_signo = signr; > - info->si_errno = 0; > info->si_code = SI_USER; > rcu_read_lock(); > info->si_pid = task_pid_vnr(current->parent); > @@ -3660,7 +3641,6 @@ static inline void prepare_kill_siginfo(int sig, struct > kernel_siginfo *info) > { > clear_siginfo(info); > info->si_signo = sig; > - info->si_errno = 0; > info->si_code = SI_USER; > info->si_pid = task_tgid_vnr(current); > info->si_uid = from_kuid_munged(current_user_ns(), current_uid()); > @@ -3833,7 +3813,6 @@ static int do_tkill(pid_t tgid, pid_t pid, int sig) > > clear_siginfo(&info); > info.si_signo = sig; > - info.si_errno = 0; > info.si_code = SI_TKILL; > info.si_pid = task_tgid_vnr(current); > info.si_uid = from_kuid_munged(current_user_ns(), current_uid());