On Sun, 31 Oct 2010, Stefan Richter wrote:
> Commit bfff68738f1c "ext4: add support for lazy inode table
> initialization" added 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
>
> This warning is due to an actual bug. But I don't know what the fix is.
>
Hi Stefan,
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.
Thanks!
-Lukas
>From 3594c17e3f724356ea8cc0a1579ab09990a771ac Mon Sep 17 00:00:00 2001
From: Lukas Czerner <[email protected]>
Date: Mon, 1 Nov 2010 15:44:54 +0100
Subject: [PATCH] ext4: fix using uninitialized variable
The ret variable might be used uninitialized. Fix this by initializing
it in definition.
Signed-off-by: "Lukas Czerner" <[email protected]>
Reported-by: "Stefan Richter" <[email protected]>
---
fs/ext4/super.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8d1d942..aff17cf 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2699,7 +2699,7 @@ static int ext4_lazyinit_thread(void *arg)
struct ext4_li_request *elr;
unsigned long next_wakeup;
DEFINE_WAIT(wait);
- int ret;
+ int ret = 0;
BUG_ON(NULL == eli);
--
1.7.2.3
Lukas Czerner wrote:
> thank you for noticing this, because I actually do not see the warning
> (I wonder why...),
Probably because of different gcc versions. Here: 4.3.4
--
Stefan Richter
-=====-==-=- =-== ----=
http://arcgraph.de/sr/
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 <[email protected]>
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" <[email protected]>
Reported-by: "Stefan Richter" <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
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))
On Tue, Nov 2, 2010 at 1:29 PM, Ted Ts'o <[email protected]> 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 <[email protected]>
> 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" <[email protected]>
> ? ?Reported-by: "Stefan Richter" <[email protected]>
> ? ?Signed-off-by: "Theodore Ts'o" <[email protected]>
>
> 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
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
On Tue, 2 Nov 2010, 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 <[email protected]>
> 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" <[email protected]>
> Reported-by: "Stefan Richter" <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
>
> 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 */
It is not removed only in the case of error, but even if it hits the
last group, so I would just omit the "error" part.
> + ext4_remove_li_request(elr);
> + continue;
> + }
> }
>
> if (time_before(elr->lr_next_sched, next_wakeup))
>
Otherwise looks good to me.
-Lukas
On Tue, 2 Nov 2010, kevin granade wrote:
> On Tue, Nov 2, 2010 at 1:29 PM, Ted Ts'o <[email protected]> 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 <[email protected]>
> > 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" <[email protected]>
> > ? ?Reported-by: "Stefan Richter" <[email protected]>
> > ? ?Signed-off-by: "Theodore Ts'o" <[email protected]>
> >
> > 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 [email protected]
> > More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at ?http://www.tux.org/lkml/
>
Thanks!
-Lukas
On 2010-11-02 15:19 -0400, Lukas Czerner wrote:
> 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 ?
This behaviour of the && operator is guaranteed by the C standard.
--
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
Lukas Czerner wrote:
> On Tue, 2 Nov 2010, kevin granade wrote:
>> + 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 ?
It is not about architecture but a C language feature. It is relied upon
everywhere in the kernel. For example,
if (p != NULL && p->m == something) {
...
is very common. The || operator has the same property: Evaluation stops as
soon as the end result is known. I do not know since when this minimum
evaluation feature is guaranteed in the language, but it has to be ages now.
(This is not an endorsement of one or the other coding of the patch. :-)
--
Stefan Richter
-=====-==-=- =-== ---=-
http://arcgraph.de/sr/
Indeed. In clause 4 of Section 6.5.13 of C99, short circuit evaluation
is guaranteed in addition to left to right evaluation.
On 11/2/10, Stefan Richter <[email protected]> wrote:
> Lukas Czerner wrote:
>> On Tue, 2 Nov 2010, kevin granade wrote:
>>> + 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 ?
>
> It is not about architecture but a C language feature. It is relied upon
> everywhere in the kernel. For example,
>
> if (p != NULL && p->m == something) {
> ...
>
> is very common. The || operator has the same property: Evaluation stops as
> soon as the end result is known. I do not know since when this minimum
> evaluation feature is guaranteed in the language, but it has to be ages now.
>
> (This is not an endorsement of one or the other coding of the patch. :-)
> --
> Stefan Richter
> -=====-==-=- =-== ---=-
> http://arcgraph.de/sr/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
Brian Gitonga Marete
Toshnix Systems
Tel: +254722151590