exit: reparent: cleanup the changing of ->parent
Hello Oleg Nesterov,
The patch eb6d8479b73d: "exit: reparent: cleanup the changing of ->parent" from Nov 13, 2014, leads to the following static checker warning:
kernel/exit.c:543 forget_original_parent()
warn: add some parenthesis here?
kernel/exit.c
538 /* Can drop and reacquire tasklist_lock */
539 reaper = find_new_reaper(father);
540 list_for_each_entry(p, &father->children, sibling) {
541 for_each_thread(p, t) {
542 t->real_parent = reaper;
543 BUG_ON(!t->ptrace != (t->parent = father));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^The reason for this warning is that many people forget that ! is higher precedence than =. This is a complicated condition however you write it, but it might be more clear to say:
BUG_ON((!!t->ptrace) = (t->parent = father));
I'm not sure.
544 if (likely(!t->ptrace))
545 t->parent = t->real_parent;
546 if (t->pdeath_signal)
547 group_send_sig_info(t->pdeath_signal,
548 SEND_SIG_NOINFO, t);
549 }
550 /*
551 * If this is a threaded reparent there is no need to 552 * notify anyone anything has happened. 553 */
554 if (!same_thread_group(reaper, father))
555 reparent_leader(father, p, &dead_children);
556 }
regards,
dan carpenter
Quoted 17 lines · Dan Carpenter
The patch
eb6d8479b73d: "exit: reparent: cleanup the changing of ->parent" from Nov 13, 2014, leads to the following static checker warning:
kernel/exit.c:543forget_original_parent()
warn: add some parenthesis here?
kernel/exit.c538 /* Can drop and reacquire tasklist_lock */ 539 reaper = find_new_reaper(father); 540 list_for_each_entry(p, &father->children, sibling) { 541 for_each_thread(p, t) { 542 t->real_parent = reaper; 543 BUG_ON(!t->ptrace != (t->parent = father)); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 2 lines
The reason for this warning is that many people forget that ! is higher precedence than =.
Do you really think we should try to shut up this warning?
IMO this warning is wrong, "!A = B" or "!A != B" looks fine to me...
Quoted 4 lines
This is a complicated condition however you write
it, but it might be more clear to say:BUG_ON((!!t->ptrace) = (t->parent = father));
This is subjective, but to me it looks more confusing.
If we really want to "fix" it, I'd suggest
BUG_ON((!t->ptrace) != (t->parent = father));
Oleg.
Quoted 18 lines · Oleg Nesterov
Quoted 17 lines · Dan Carpenter
The patch
eb6d8479b73d: "exit: reparent: cleanup the changing of ->parent" from Nov 13, 2014, leads to the following static checker warning:
kernel/exit.c:543forget_original_parent()
warn: add some parenthesis here?
kernel/exit.c538 /* Can drop and reacquire tasklist_lock */ 539 reaper = find_new_reaper(father); 540 list_for_each_entry(p, &father->children, sibling) { 541 for_each_thread(p, t) { 542 t->real_parent = reaper; 543 BUG_ON(!t->ptrace != (t->parent = father)); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 7 lines
Quoted 2 lines
The reason for this warning is that many people forget that ! is higher precedence than =.
Do you really think we should try to shut up this warning?
IMO this warning is wrong, "!A = B" or "!A != B" looks fine to me...
Most of the time when Smatch prints a warning about those then it's a bug. It's a mix of precedence bugs and adding accidental negates.
I'll send a patch to put some parenthesis around the negate.
regards,
dan carpenter