Staging: lustre: Convert array index from the loop bound to the loop index.
PATCH 1/1 Staging: lustre: Convert array index from the loop bound to the loop index.
Convert array index from the loop bound to the loop index. Used below semantic patch to identify this issue:
@@
expression e1,e2,ar;
@@
for(e1 = 0; e1 < e2; e1++) { <...
ar[
- e2
+ e1
]
...> }
--- drivers/staging/lustre/lustre/obdclass/cl_object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/staging/lustre/lustre/obdclass/cl_object.c +1 -1
@@ -1000,7 +1000,7 @@ static int cl_env_percpu_init(void)* thus we must uninitialize up to i, the rest are undefined. */ for (j =0 ; j < i; j++) { - cle = &cl_env_percpu[i]; + cle = &cl_env_percpu[j];lu_context_exit (&cle->ce_ses);lu_context_fini (&cle->ce_ses);lu_env_fini (&cle->ce_lu);
Signature · 1 line
1.7.1
Quoted 2 lines · Sandhya Bankar
Convert array index from the loop bound to the loop index. Used below semantic patch to identify this issue:
This is a really nice idea! However, it would be good for the commit message to explain why the change is correct. Certainly the code is suspicious, but why is it actually wrong? The idea would be to consider what is done with cle on each iteration.
julia
Quoted 40 lines
@@
expression e1,e2,ar;
@@for(e1 = 0; e1 < e2; e1++) { <...
ar[
- e2
+ e1
]
...> }Signed-off-by Sandhya Bankar <[email protected]>--- drivers/staging/lustre/lustre/obdclass/cl_object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/staging/lustre/lustre/obdclass/cl_object.c +1
-2@@ -1000,7 +1000,7 @@ static int cl_env_percpu_init(void)* thus we must uninitialize up to i, the rest are undefined. */ for (j =0 ; j < i; j++) { - cle = &cl_env_percpu[i]; + cle = &cl_env_percpu[j];lu_context_exit (&cle->ce_ses);lu_context_fini (&cle->ce_ses);lu_env_fini (&cle->ce_lu); --1.7.1
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160915194341.GA30843%40sandhya.
For more options, visit https://groups.google.com/d/optout.
The structure cl_env_percpu[NR_CPUS] have been initializing for each possible cpu (i.e 0 to i) but during
initialization if any error will occurred this come to the error handling code in which the cl_env_percpu should be uninitialized upto i but currently uninitialization happening for the cl_env_percpu[i] which does not seems to be correct. That is why proposing this fix. Please Correct me ,If I am wrong. Should I send patch v2 with this issue description ?
Quoted 60 lines · Julia Lawall
Quoted 2 lines · Sandhya Bankar
Convert array index from the loop bound to the loop index. Used below semantic patch to identify this issue:
This is a really nice idea! However, it would be good for the commit message to explain why the change is correct. Certainly the code is suspicious, but why is it actually wrong? The idea would be to consider what is done with cle on each iteration.
julia
Quoted 12 lines
@@
expression e1,e2,ar;
@@for(e1 = 0; e1 < e2; e1++) { <...
ar[
- e2
+ e1
]
...> }Review
Quoted 6 lines
Signed-off-by Sandhya Bankar <[email protected]>--- drivers/staging/lustre/lustre/obdclass/cl_object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)(quoted diff) +0
-0
b/drivers/staging/lustre/lustre/obdclass/cl_object.cdrivers/staging/lustre/lustre/obdclass/cl_object.c static int cl_env_percpu_init(void) · line 1000
Quoted 5 lines
index 91a5806..eeccd30 100755
drivers/staging/lustre/lustre/obdclass/cl_object.c +0
-0@@ -1000,7 +1000,7 @@ static int cl_env_percpu_init(void)* thus we must uninitialize up to i, the rest are undefined.
Quoted 10 lines
drivers/staging/lustre/lustre/obdclass/cl_object.c +1
-2*/ for (j =0 ; j < i; j++) { - cle = &cl_env_percpu[i]; + cle = &cl_env_percpu[j];lu_context_exit (&cle->ce_ses);lu_context_fini (&cle->ce_ses);lu_env_fini (&cle->ce_lu); --1.7.1
Quoted 2 lines
--
You received this message because you are subscribed to the GoogleGroups "outreachy-kernel" group.
Quoted 1 line
To unsubscribe from this group and stop receiving emails from it, send
an email to [email protected].
Quoted 2 lines
To post to this group, send email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/
msgid/outreachy-kernel/20160915194341.GA30843%40sandhya.
Quoted 2 lines
For more options, visit https://groups.google.com/d/optout.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · Andreas
Quoted 7 lines · sandhya bankar
The structure cl_env_percpu[NR_CPUS] have been initializing for each possible cpu (i.e 0 to i) but during initialization if any error will occurred this come to the error handling code in which the
cl_env_percpushould be uninitialized upto i but currently
Sandhya,
"should be uninitialized" is a bit unclear, because it could describe the state that should hold, or the state that you should achieve. It could be better to say "the error handling code which should uninitialize/free/release (whatever seems best) the cl_env_percpu ...".
Andreas's suggestion is also good. It says that same thing in another way, and thus makes everything more clear.
Congratulations on finding a real bug :)
julia
Quoted 71 lines
Quoted 3 lines
uninitialization happening for the cl_env_percpu[i] which does not seems to be correct. That is why proposing this fix. Please Correct me ,If I am wrong. Should I send patch v2 with this issue description ?
This is definitely a bug, though thankfully only in an error handling path.
Your description is much more clear. You might want to write
"... but currently cleanup is happening repeatedly for the same
cl_env_percpu[i] element."Cheers, Andreas
Quoted 56 lines · Julia Lawall
Quoted 55 lines · Sandhya Bankar
Quoted 3 lines
Convert array index from the loop bound to the loop index. Used below semantic patch to identify this issue:
This is a really nice idea! However, it would be good for the commit message to explain why the change is correct. Certainly the code is suspicious, but why is it actually wrong? The idea would be to consider what is done with cle on each iteration.
julia
Quoted 40 lines
@@
expression e1,e2,ar;
@@for(e1 = 0; e1 < e2; e1++) { <...
ar[
- e2
+ e1
]
...> }Signed-off-by Sandhya Bankar <[email protected]>--- drivers/staging/lustre/lustre/obdclass/cl_object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/staging/lustre/lustre/obdclass/cl_object.c +1
-2@@ -1000,7 +1000,7 @@ static int cl_env_percpu_init(void)* thus we must uninitialize up to i, the rest are undefined. */ for (j =0 ; j < i; j++) { - cle = &cl_env_percpu[i]; + cle = &cl_env_percpu[j];lu_context_exit (&cle->ce_ses);lu_context_fini (&cle->ce_ses);lu_env_fini (&cle->ce_lu); --1.7.1
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160915194341.GA30843%40sandhya.
For more options, visit https://groups.google.com/d/optout.
Thanks Julia and Andreas.
Should I need to send patch v2 as per your suggestions ?
Quoted 109 lines · Julia Lawall
Quoted 2 lines · Andreas
Quoted 2 lines
On Sep 15, 2016, at 14:53, sandhya bankar <[email protected]>
Quoted 5 lines · wrote:
Quoted 5 lines
The structure cl_env_percpu[NR_CPUS] have been initializing for each possible cpu (i.e 0 to i) but during initialization if any error will occurred this come to the error handling code in which the
cl_env_percpushould be uninitialized upto i but currentlySandhya,
"should be uninitialized" is a bit unclear, because it could describe the state that should hold, or the state that you should achieve. It could be better to say "the error handling code which should uninitialize/free/release (whatever seems best) the
cl_env_percpu...".Andreas's suggestion is also good. It says that same thing in another way, and thus makes everything more clear.
Congratulations on finding a real bug :)
julia
Quoted 5 lines
Quoted 3 lines
uninitialization happening for the cl_env_percpu[i] which does not seems to be correct. That is why proposing this fix. Please Correct me ,If I am wrong. Should I send patch v2 with this issue description ?
This is definitely a bug, though thankfully only in an error handling
path.
Quoted 9 lines
Your description is much more clear. You might want to write
"... but currently cleanup is happening repeatedly for the same
cl_env_percpu[i] element."Cheers, Andreas
Quoted 1 line
On Fri, Sep 16, 2016 at 1:35 AM, Julia Lawall <[email protected]>
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 10 lines · Sandhya Bankar
Quoted 3 lines
Convert array index from the loop bound to the loop index. Used below semantic patch to identify this issue:
This is a really nice idea! However, it would be good for the commit message to explain why the change is correct. Certainly the code is suspicious, but why is it actually wrong? The idea would be to
consider
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
what is done with cle on each iteration.
julia
Quoted 12 lines
@@
expression e1,e2,ar;
@@for(e1 = 0; e1 < e2; e1++) { <...
ar[
- e2
+ e1
]
...> }Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Review
Quoted 6 lines
Signed-off-by Sandhya Bankar <[email protected]>--- drivers/staging/lustre/lustre/obdclass/cl_object.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)(quoted diff) +0
-0
b/drivers/staging/lustre/lustre/obdclass/cl_object.cQuoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/staging/lustre/lustre/obdclass/cl_object.c static int cl_env_percpu_init(void) · line 1000
Quoted 5 lines
index 91a5806..eeccd30 100755
drivers/staging/lustre/lustre/obdclass/cl_object.c +0
-0@@ -1000,7 +1000,7 @@ static int cl_env_percpu_init(void)* thus we must uninitialize up to i, the rest are undefined.
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/staging/lustre/lustre/obdclass/cl_object.c +1
-2*/ for (j =0 ; j < i; j++) { - cle = &cl_env_percpu[i]; + cle = &cl_env_percpu[j];lu_context_exit (&cle->ce_ses);lu_context_fini (&cle->ce_ses);lu_env_fini (&cle->ce_lu); --1.7.1
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
--
You received this message because you are subscribed to the GoogleGroups "outreachy-kernel" group.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
To unsubscribe from this group and stop receiving emails from it,
send an email to [email protected].
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
To post to this group, send email to outreachy-kernel@googlegroups.
com.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
To view this discussion on the web visit
https://groups.google.com/d/msgid/outreachy-kernel/20160915194341.GA30843%
40sandhya.Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
For more options, visit https://groups.google.com/d/optout.
Quoted 2 lines · sandhya bankar
Thanks Julia and Andreas.
Should I need to send patch v2 as per your suggestions ?
Yes.
julia
Quoted 140 lines · Julia Lawall
On Thu, 15 Sep 2016, Dilger, Andreas wrote: > > > On Sep 15, 2016, at 14:53, sandhya bankar <[email protected]> wrote: > > > > The structure cl_env_percpu[NR_CPUS] have been initializing for each > > possible cpu (i.e 0 to i) but during initialization if any error will > > occurred this come to the error handling code in which the > > cl_env_percpu should be uninitialized upto i but currently Sandhya, "should be uninitialized" is a bit unclear, because it could describe the state that should hold, or the state that you should achieve. It could be better to say "the error handling code which should uninitialize/free/release (whatever seems best) the cl_env_percpu ...". Andreas's suggestion is also good. It says that same thing in another way, and thus makes everything more clear. Congratulations on finding a real bug :) julia > > uninitialization happening for the cl_env_percpu[i] which does not > > seems to be correct. That is why proposing this fix. Please Correct me > > ,If I am wrong. Should I send patch v2 with this issue description ? > > This is definitely a bug, though thankfully only in an error handling path. > > Your description is much more clear. You might want to write > > "... but currently cleanup is happening repeatedly for the same > cl_env_percpu[i] element." > > Cheers, Andreas > > > On Fri, Sep 16, 2016 at 1:35 AM, Julia Lawall <[email protected]> wrote: > >> > >> > >> On Fri, 16 Sep 2016, Sandhya Bankar wrote: > >> > >> > Convert array index from the loop bound to the loop index. > >> > Used below semantic patch to identify this issue: > >> > >> This is a really nice idea! However, it would be good for the commit > >> message to explain why the change is correct. Certainly the code is > >> suspicious, but why is it actually wrong? The idea would be to consider > >> what is done with cle on each iteration. > >> > >> julia > >> > >> > @@ > >> > expression e1,e2,ar; > >> > @@ > >> > > >> > for(e1 = 0; e1 < e2; e1++) { <... > >> > ar[ > >> > - e2 > >> > + e1 > >> > ] > >> > ...> } > >> > > >> > > >> > Signed-off-by: Sandhya Bankar <[email protected]> > >> > --- > >> > drivers/staging/lustre/lustre/obdclass/cl_object.c | 2 +- > >> > 1 files changed, 1 insertions(+), 1 deletions(-) > >> > > >> > diff --git a/drivers/staging/lustre/lustre/obdclass/cl_object.c b/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> > index 91a5806..eeccd30 100755 > >> > --- a/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> > +++ b/drivers/staging/lustre/lustre/obdclass/cl_object.c > >> > @@ -1000,7 +1000,7 @@ static int cl_env_percpu_init(void) > >> > * thus we must uninitialize up to i, the rest are undefined. > >> > */ > >> > for (j = 0; j < i; j++) { > >> > - cle = &cl_env_percpu[i]; > >> > + cle = &cl_env_percpu[j]; > >> > lu_context_exit(&cle->ce_ses); > >> > lu_context_fini(&cle->ce_ses); > >> > lu_env_fini(&cle->ce_lu); > >> > -- > >> > 1.7.1 > >> > > >> > -- > >> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > >> > To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. > >> > To post to this group, send email to [email protected]. > >> > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/20160915194341.GA30843%4 0sandhya. > >> > For more options, visit https://groups.google.com/d/optout. > >> > > >> > >--
You received this message because you are subscribed to the Google Groups
"outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/CAA3ASz%3Dt4wZOkk%3DBj8i YLMFHwzCj%2BE%2B0BbC9UsV%2BL17oToh3ZA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.