From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751786AbYLOLw5 (ORCPT ); Mon, 15 Dec 2008 06:52:57 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750902AbYLOLwt (ORCPT ); Mon, 15 Dec 2008 06:52:49 -0500 Received: from mx2.redhat.com ([66.187.237.31]:52143 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750855AbYLOLws (ORCPT ); Mon, 15 Dec 2008 06:52:48 -0500 Date: Mon, 15 Dec 2008 12:51:08 +0100 From: Oleg Nesterov To: Jiri Slaby Cc: kenchen@google.com, Linux kernel mailing list , "Eric W. Biederman" Subject: Re: broken do_each_pid_{thread,task} Message-ID: <20081215115108.GA17577@redhat.com> References: <494581D7.6000203@gmail.com> <20081215102415.GA11106@redhat.com> <49463679.7080306@gmail.com> <20081215110238.GA15606@redhat.com> <4946406D.6000506@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4946406D.6000506@gmail.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.