Messages in this thread | | | Date | Mon, 15 Dec 2008 12:51:08 +0100 | From | Oleg Nesterov <> | Subject | Re: broken do_each_pid_{thread,task} |
| |
On 12/15, Jiri Slaby wrote: > > Oleg Nesterov napsal(a): > > On 12/15, Jiri Slaby wrote: > >> Oleg Nesterov napsal(a): > >>> On 12/14, Jiri Slaby wrote: > >>>> I'm getting > >>>> `if (type == PIDTYPE_PID)' is unreachable > >>>> warning from kernel/exit.c. The preprocessed code looks like: > >>>> do { > >>>> struct hlist_node *pos___; > >>>> if (pgrp != ((void *)0)) > >>>> for (LIST ITERATION) { > >>>> { > >>>> if (!((p->state & 4) != 0)) > >>>> continue; > >>>> retval = 1; > >>>> break; > >>>> } > >>>> if (PIDTYPE_PGID == PIDTYPE_PID) > >>>> break; > >>>> } > >>>> } while (0); > >>>> and it's obviously wrong. > >>> Why do you think it is wrong? This break stops the "hlist_for_each" > >>> loop, not the enclosing "do while". > >> The `continue' matters here (and also in other do_each_pid_task cases). > >> Sorry for not mentioning it explicitly. > > > > Still can't understand... OK, I think we misundersood each other. > > Do you agree that the code is technically correct? Or I missed > > something? > > > > "continue" looks fine to me too, it is also for the inner loop. > > But it doesn't jump to the `if' (this is what I would expect from the > `continue' here), but to the third statement of the `for'.
Yes, but this doesn't matter,
> Maybe better to ask, is the test expected to be fired after *each* > invocation of the body?
Yes, but we need this only when type == PIDTYPE_PID, so the code is correct.
> > Look, "if (PIDTYPE_PGID == PIDTYPE_PID)" is not possible too, should > > the compiler (or whatever) complain? > > Correct, in this particular case (and I checked that also other users which > uses `continue' inside the loop don't pass PIDTYPE_PID).
Yes.
Don't get me wrong, I do agree (let me repeat again) this check is absolutely ugly and may cause the problems.
As I said, it fixes the minor and only theoretical problem. Perhaps we can move this check to the code which does do_each_pid_task(PIDTYPE_PID). Or better yet, just introduce for_each_pid_task() (see another email).
> >> (And it's not compiler which complains > >> here.) > > > > Ah, OK, thanks. Just curious, and who does? > > A static analyzer. Stay tuned, we will announce it later, it's in the state > of development :).
Great, thanks ;)
Oleg.
| |