Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755026Ab0KBTT2 (ORCPT ); Tue, 2 Nov 2010 15:19:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22324 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752357Ab0KBTTW (ORCPT ); Tue, 2 Nov 2010 15:19:22 -0400 Date: Tue, 2 Nov 2010 15:19:09 -0400 (EDT) From: Lukas Czerner X-X-Sender: lukas@dhcp-lab-213.englab.brq.redhat.com To: kevin granade cc: "Ted Ts'o" , Lukas Czerner , Stefan Richter , linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function In-Reply-To: Message-ID: References: <20101102182911.GE25614@thunk.org> User-Agent: Alpine 2.00 (LFD 1167 2008-08-23) MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="-1463809279-2035809424-1288725551=:16869" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4796 Lines: 118 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1463809279-2035809424-1288725551=:16869 Content-Type: TEXT/PLAIN; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT On Tue, 2 Nov 2010, kevin granade wrote: > On Tue, Nov 2, 2010 at 1:29 PM, Ted Ts'o wrote: > > > > On Mon, Nov 01, 2010 at 04:27:26PM +0100, Lukas Czerner wrote: > > > > > > thank you for noticing this, because I actually do not see the warning > > > (I wonder why...), but it is definitely a bug, so the trivial patch below > > > should fix that. > > > > This is a slightly less trivial fix that eliminates the need for the > > "ret" variable entirely. > > > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?- Ted > > > > commit e048924538f0c62d18306e2fea0e22dac0140f6e > > Author: Theodore Ts'o > > Date: ? Tue Nov 2 14:19:30 2010 -0400 > > > > ? ?ext4: "ret" may be used uninitialized in ext4_lazyinit_thread() > > > > ? ?Newer GCC's reported the following build warning: > > > > ? ? ? fs/ext4/super.c: In function 'ext4_lazyinit_thread': > > ? ? ? fs/ext4/super.c:2702: warning: 'ret' may be used uninitialized in this function > > > > ? ?Fix it by removing the need for the ret variable in the first place. > > > > ? ?Signed-off-by: "Lukas Czerner" > > ? ?Reported-by: "Stefan Richter" > > ? ?Signed-off-by: "Theodore Ts'o" > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index 8d1d942..4d7ef31 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -2699,7 +2699,6 @@ static int ext4_lazyinit_thread(void *arg) > > ? ? ? ?struct ext4_li_request *elr; > > ? ? ? ?unsigned long next_wakeup; > > ? ? ? ?DEFINE_WAIT(wait); > > - ? ? ? int ret; > > > > ? ? ? ?BUG_ON(NULL == eli); > > > > @@ -2723,13 +2722,12 @@ cont_thread: > > ? ? ? ? ? ? ? ? ? ? ? ?elr = list_entry(pos, struct ext4_li_request, > > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? lr_request); > > > > - ? ? ? ? ? ? ? ? ? ? ? if (time_after_eq(jiffies, elr->lr_next_sched)) > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ret = ext4_run_li_request(elr); > > - > > - ? ? ? ? ? ? ? ? ? ? ? if (ret) { > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ret = 0; > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ext4_remove_li_request(elr); > > - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? continue; > > + ? ? ? ? ? ? ? ? ? ? ? if (time_after_eq(jiffies, elr->lr_next_sched)) { > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (ext4_run_li_request(elr) != 0) { > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* error, remove the lazy_init job */ > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ext4_remove_li_request(elr); > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? continue; > > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } > > ? ? ? ? ? ? ? ? ? ? ? ?} > > > > ? ? ? ? ? ? ? ? ? ? ? ?if (time_before(elr->lr_next_sched, next_wakeup)) > > What do you think about this option for the second hunk? (not anything-tested) > > @@ -2723,13 +2722,11 @@ cont_thread: > elr = list_entry(pos, struct ext4_li_request, > lr_request); > - if (time_after_eq(jiffies, elr->lr_next_sched)) > - ret = ext4_run_li_request(elr); > - > - if (ret) { > - ret = 0; > - ext4_remove_li_request(elr); > - continue; > + if (time_after_eq(jiffies, elr->lr_next_sched) && > + ext4_run_li_request(elr) != 0) { > + /* error, remove the lazy_init job */ > + ext4_remove_li_request(elr); > + continue; > } > > if (time_before(elr->lr_next_sched, next_wakeup)) > -- > > Though obviously it's a pretty subjective style issue. > Kevin Granade Hmm this relies on the fact that if the first part of the condition would not be true, the second part (after and) would never be invoked, however I am not really sure that we can rely on that on every architecture, or can we ? > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at ?http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at ?http://www.tux.org/lkml/ > Thanks! -Lukas ---1463809279-2035809424-1288725551=:16869-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/