re: exit: reparent: cleanup the changing of ->parent

re: exit: reparent: cleanup the changing of ->parent

[Dan Carpenter]

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

Re: exit: reparent: cleanup the changing of ->parent

[Oleg Nesterov]

On 11/14, Dan Carpenter wrote:
>
> 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 =.

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...


> 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.

Re: exit: reparent: cleanup the changing of ->parent

[Dan Carpenter]

On Mon, Nov 17, 2014 at 07:42:28PM +0100, Oleg Nesterov wrote:
> On 11/14, Dan Carpenter wrote:
> >
> > 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 =.
> 
> 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