Staging: lustre: Convert array index from the loop bound to the loop index.

PATCH Superseded Sandhya Bankar 2 participants 6 messages Sep 15, 2016 – Sep 16, 2016 1 message not in the archive Staging

lore

Share & tools

lore

Sandhya Bankar

author

2016-09-15 19:43 UTC #0 lore raw

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
]
...> }

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 -1

index 91a5806..eeccd30 100755
@@ -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

Julia Lawall

2016-09-15 20:05 UTC #1 lore raw

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
index 91a5806..eeccd30 100755
@@ -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.

sandhya bankar

author

2016-09-15 20:53 UTC #2 lore raw

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.c
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
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 Google
Groups "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.

Julia Lawall

2016-09-16 05:47 UTC #4 lore raw

In reply to a message not in the archive

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_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

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
index 91a5806..eeccd30 100755
@@ -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.

sandhya bankar

author

2016-09-16 08:35 UTC #5 lore raw

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_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
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.c
Quoted 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 Google
Groups "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.

Julia Lawall

2016-09-16 08:38 UTC #6 lore raw

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.