[PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Subrata Modak]

Following fix is inspired by David Howells fix few days back:
http://lkml.org/lkml/2009/7/9/109,

Signed-off-by: Subrata Modak<subrata@linux.vnet.ibm.com>,
---

--- a/fs/ubifs/commit.c	2009-05-20 10:44:12.000000000 +0530
+++ b/fs/ubifs/commit.c	2009-07-15 05:39:06.000000000 +0530
@@ -510,7 +510,7 @@ int dbg_check_old_index(struct ubifs_inf
 	int lnum, offs, len, err = 0, uninitialized_var(last_level), child_cnt;
 	int first = 1, iip;
 	struct ubifs_debug_info *d = c->dbg;
-	union ubifs_key lower_key, upper_key, l_key, u_key;
+	union ubifs_key uninitialized_var(lower_key), upper_key, l_key, u_key;
 	unsigned long long uninitialized_var(last_sqnum);
 	struct ubifs_idx_node *idx;
 	struct list_head list;

---
Regards--
Subrata

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Artem Bityutskiy]

On Wed, 2009-07-15 at 07:49 +0530, Subrata Modak wrote:
> Following fix is inspired by David Howells fix few days back:
> http://lkml.org/lkml/2009/7/9/109,
> 
> Signed-off-by: Subrata Modak<subrata@linux.vnet.ibm.com>,
> ---

Removed junk comma at the end of "signed-off-by" and pushed to
the ubifs-2.6.git tree:

http://git.infradead.org/ubifs-2.6.git?a=commit;h=5c1507e6097c4abc13bbad69de137366c9043f22

Thanks.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Stefan Richter]

Artem Bityutskiy wrote:
> On Wed, 2009-07-15 at 07:49 +0530, Subrata Modak wrote:
>> Following fix is inspired by David Howells fix few days back:
>> http://lkml.org/lkml/2009/7/9/109,
>>
>> Signed-off-by: Subrata Modak<subrata@linux.vnet.ibm.com>,
>> ---
> 
> Removed junk comma at the end of "signed-off-by" and pushed to
> the ubifs-2.6.git tree:
> 
> http://git.infradead.org/ubifs-2.6.git?a=commit;h=5c1507e6097c4abc13bbad69de137366c9043f22
> 
> Thanks.
> 

The changelog of the patch is bad.  "Fix compilation warning" is not 
correct.  It should be "suppress compilation warning" or "annotate 
unitialized variable" or whatever --- i.e. it should say what it does. 
Furthermore, since the 3 lines context around the change in the diff do 
not reveal why the chosen "fix" is correct and desirable, the changelog 
should also leave a note why it's done this way.

The patch form David Howells which is quoted here has an equally bad 
subject, but at least its changelog goes on to explain what the patch 
really does and why it does it in the proposed way.
-- 
Stefan Richter
-=====-==--= -=== -====
http://arcgraph.de/sr/

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Artem Bityutskiy]

On Wed, 2009-07-15 at 20:16 +0200, Stefan Richter wrote:
> Artem Bityutskiy wrote:
> > On Wed, 2009-07-15 at 07:49 +0530, Subrata Modak wrote:
> >> Following fix is inspired by David Howells fix few days back:
> >> http://lkml.org/lkml/2009/7/9/109,
> >>
> >> Signed-off-by: Subrata Modak<subrata@linux.vnet.ibm.com>,
> >> ---
> > 
> > Removed junk comma at the end of "signed-off-by" and pushed to
> > the ubifs-2.6.git tree:
> > 
> > http://git.infradead.org/ubifs-2.6.git?a=commit;h=5c1507e6097c4abc13bbad69de137366c9043f22
> > 
> > Thanks.
> > 
> 
> The changelog of the patch is bad.  "Fix compilation warning" is not 
> correct.  It should be "suppress compilation warning" or "annotate 
> unitialized variable" or whatever --- i.e. it should say what it does.

For me this sounds the same. But probably your version is better
English. I'll change this.

> Furthermore, since the 3 lines context around the change in the diff do 
> not reveal why the chosen "fix" is correct and desirable, the changelog 
> should also leave a note why it's done this way.

The changelog says which kind of warning is fixed, I though it is
obvious what is the warning. At lease for me it would.

But if Subrata sends me the warning he sees, I'll change that.
Thankfully I did not push the patch to ubifs-2.6.git/linux-next
which I never re-base, but pushed it to master which I do rebase
and it is documented here:
http://www.linux-mtd.infradead.org/doc/ubifs.html#L_source

So I may just amend the commit's message.

> The patch form David Howells which is quoted here has an equally bad 
> subject, but at least its changelog goes on to explain what the patch 
> really does and why it does it in the proposed way.

Well, I just thought this type of warnings and way of fixing is very
standard because I saw many similar fixes all over the place.

Anyway, amended the patch like this so far:
http://git.infradead.org/ubifs-2.6.git?a=commit;h=5c1507e6097c4abc13bbad69de137366c9043f22

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Subrata Modak]

Hi,

On Thu, 2009-07-16 at 12:57 +0300, Artem Bityutskiy wrote:
> On Wed, 2009-07-15 at 20:16 +0200, Stefan Richter wrote:
> > Artem Bityutskiy wrote:
> > > On Wed, 2009-07-15 at 07:49 +0530, Subrata Modak wrote:
> > >> Following fix is inspired by David Howells fix few days back:
> > >> http://lkml.org/lkml/2009/7/9/109,
> > >>
> > >> Signed-off-by: Subrata Modak<subrata@linux.vnet.ibm.com>,
> > >> ---
> > > 
> > > Removed junk comma at the end of "signed-off-by" and pushed to
> > > the ubifs-2.6.git tree:
> > > 
> > > http://git.infradead.org/ubifs-2.6.git?a=commit;h=5c1507e6097c4abc13bbad69de137366c9043f22
> > > 
> > > Thanks.
> > > 
> > 
> > The changelog of the patch is bad.  "Fix compilation warning" is not 
> > correct.  It should be "suppress compilation warning" or "annotate 
> > unitialized variable" or whatever --- i.e. it should say what it does.

Ok, i would change accordingly.

> 
> For me this sounds the same. But probably your version is better
> English. I'll change this.
> 
> > Furthermore, since the 3 lines context around the change in the diff do 
> > not reveal why the chosen "fix" is correct and desirable, the changelog 
> > should also leave a note why it's done this way.
> 
> The changelog says which kind of warning is fixed, I though it is
> obvious what is the warning. At lease for me it would.
> 
> But if Subrata sends me the warning he sees, I'll change that.
> Thankfully I did not push the patch to ubifs-2.6.git/linux-next
> which I never re-base, but pushed it to master which I do rebase
> and it is documented here:
> http://www.linux-mtd.infradead.org/doc/ubifs.html#L_source

I would resend with exact warning generated, etc.

> 
> So I may just amend the commit's message.
> 
> > The patch form David Howells which is quoted here has an equally bad 
> > subject, but at least its changelog goes on to explain what the patch 
> > really does and why it does it in the proposed way.

Well, untill gcc becomes a little more intelligent, i believe people
would continue to fix them like this way. I would add proper description
in my resend patch.

> 
> Well, I just thought this type of warnings and way of fixing is very
> standard because I saw many similar fixes all over the place.
> 

Correct. There has been other warning fixes i have sent to LKML, where i
have tweaked the code to fix the compilation, but, code tweaking may not
be possible in this case. However , i would  still investigate.

Regards--
Subrata

> Anyway, amended the patch like this so far:
> http://git.infradead.org/ubifs-2.6.git?a=commit;h=5c1507e6097c4abc13bbad69de137366c9043f22
> 

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Artem Bityutskiy]

On Thu, 2009-07-16 at 16:34 +0530, Subrata Modak wrote:
> Correct. There has been other warning fixes i have sent to LKML, where i
> have tweaked the code to fix the compilation, but, code tweaking may not
> be possible in this case. However , i would  still investigate.

I do not think that in _general_ it is a good idea to tweak code just
to have gcc pleased, granted the code is correct. In this case it is
more preferable to shut it up, e.g., using 'unitialized_var()'.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Subrata Modak]

On Thu, 2009-07-16 at 14:12 +0300, Artem Bityutskiy wrote:
> On Thu, 2009-07-16 at 16:34 +0530, Subrata Modak wrote:
> > Correct. There has been other warning fixes i have sent to LKML, where i
> > have tweaked the code to fix the compilation, but, code tweaking may not
> > be possible in this case. However , i would  still investigate.
> 
> I do not think that in _general_ it is a good idea to tweak code just
> to have gcc pleased, granted the code is correct. In this case it is
> more preferable to shut it up, e.g., using 'unitialized_var()'.

Great. I would write more description in the patch then. Thanks.

Regards--
Subrata

> 

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Stefan Richter]

Artem Bityutskiy wrote:
> On Wed, 2009-07-15 at 20:16 +0200, Stefan Richter wrote:
>> The changelog of the patch is bad.  "Fix compilation warning" is not 
>> correct.  It should be "suppress compilation warning" or "annotate 
>> unitialized variable" or whatever --- i.e. it should say what it does.
> 
> For me this sounds the same. But probably your version is better
> English. I'll change this.
> 
>> Furthermore, since the 3 lines context around the change in the diff do 
>> not reveal why the chosen "fix" is correct and desirable, the changelog 
>> should also leave a note why it's done this way.
> 
> The changelog says which kind of warning is fixed, I though it is
> obvious what is the warning. At lease for me it would.
> 
> But if Subrata sends me the warning he sees, I'll change that.
> Thankfully I did not push the patch to ubifs-2.6.git/linux-next
> which I never re-base, but pushed it to master which I do rebase
> and it is documented here:
> http://www.linux-mtd.infradead.org/doc/ubifs.html#L_source
> 
> So I may just amend the commit's message.
> 
>> The patch form David Howells which is quoted here has an equally bad 
>> subject, but at least its changelog goes on to explain what the patch 
>> really does and why it does it in the proposed way.
> 
> Well, I just thought this type of warnings and way of fixing is very
> standard because I saw many similar fixes all over the place.
> 
> Anyway, amended the patch like this so far:
> http://git.infradead.org/ubifs-2.6.git?a=commit;h=5c1507e6097c4abc13bbad69de137366c9043f22

Hi,

my comment was more directed to Subrata rather than you, as a pointer 
for future changes of this kind.

By '"Fix compilation warning" is not correct' I mean:

This subject would be strictly correct if there was a warning message 
which contained a factual or formal mistake, and the patch would make 
the warning show up as a factual and formal correct message.

Yet the patch does nothing else than _suppress a message_.

This comment surely sounds pedantic, but my motive for posting was that 
changes which are posted with a sloppily written (and in this case, 
incomplete) changelog are too often ill-conceived themselves.

The warning which is being suppressed here is surely of the kind 
"variable may be used uninitialized".

There are of course different possible causes for such a warning to come 
up --- and depending on the cause, different types of "fixes" are called 
for.

     1.) Reason:  A variable is in fact used uninitialized; maybe only in
         border cases which are extremely rarely executed in practice.

         Wrong "fix":  Shut up the compiler with this uninitialized_var()
         macro (or worse, by an arbitrary dummy initialization).  Then
         the bug is still there, it only has become invisible, which is
         of course even worse.

         Right fix:  Fix up the code to provide the intended value in the
         variable.

     2.) Reason:  The compiler wrongly interprets a variable as
         occasionally unused, because there is convoluted, messy code.

         Wrong "fix":  Shut up the compiler with this uninitialized_var()
         macro or worse, by an initialization.  Then the code has become
         even messier than it already was!

         Right fix:  Refactor the code to become cleanly laid out, easy
         to follow, obviously correct.  (And as a side effect, compilable
         without warning.  But that's only a side effect, not the actual
         goal of the code change!)

     3.) Reason:  The compiler wrongly interprets a variable as
         occasionally unused, even though this is nicely organized,
         obvious code where the assertion of the compiler is not only
         wrong, but everybody also can see immediately from the code that
         there will never be a.

         Aside from improving the compiler itself to properly handle
         this, in this case the appropriate action is to use the
         uninitialized_var() macro --- /unless/ there is a non-intrusive
         way to refactor the code to be compilable without warning but
         keep it still nice to read.

         Why is the latter still better than uninitialized_var()?  Easy:
         If anybody later changes the code such that he introduces an
         actual use-without-initialization bug, nobody will notice the
         bug because it's hidden by the macro.

         And there are more downsides to the macro:  The readability of
         code which makes use of it suffers somewhat.  Also, the macro
         works with gcc (i.e. shuts gcc up), but apparently not with the
         Intel compiler.

So, in the majority of types of cases, uninitialized_var() is the wrong 
course of action.  (Though maybe not in the majority of occurrences of 
cases.)

Furthermore, Subrata's changelog does not document whether there was a 
deeper analysis of the real reason for the warning and for the most 
appropriate solution.  The changelog only points to uninitialized_var() 
usage in entirely unrelated code.

OK.  Rant over.  It's now time for me to actually have look at the code 
which is being patched here. :-)  (Sorry, I commented before only on the 
basis of Subrata's diff and changelog.)

 From fs/ubifs/commit.c in mainline 2.6.31-rc3:

Before lower_key, there was obviously already an issue with last_sqnum 
which received the same treatment as lower_key receives now.  Both of 
these are provided with a value in a conditional block:

	int first = 1;

	if (first) {
		first = 0;
		...
	}

block and first used after this block.  From the looks of the whole 
function, these two variables indeed don't appear in danger to ever be 
accessed uninitialized.

So, is uninitialized_var() a good fix here?  I'd say it's not a 
particular good one.  Count the lines of code of dbg_check_old_index() 
and the maximum indentation level of it.  Then remember the teachings of 
CodingStyle. :-)  See?  dbg_check_old_index() clearly isn't a prime 
example of best kernel coding practice.  /Perhaps/ a little bit of 
refactoring would make it easier to read, and as a bonus side effect, 
make it unnecessary to use the slightly dangerous and 
uninitialized_var() macro here.
-- 
Stefan Richter
-=====-==--= -=== =----
http://arcgraph.de/sr/

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Stefan Richter]

Subrata Modak wrote:
>> Well, I just thought this type of warnings and way of fixing is very
>> standard because I saw many similar fixes all over the place.
> 
> Correct. There has been other warning fixes i have sent to LKML, where i
> have tweaked the code to fix the compilation, but, code tweaking may not
> be possible in this case.

Wrong goal.  The goal of a patch should be to improve the code.

To remove false-positive warnings from compilation can only be a 
secondary goal.

(See my other post.)

BTW, which compiler do you use?  I quickly enabled ubifs here, first 
without and then with its various sub-uptions, but didn't get a warning 
here with "gcc (Gentoo 4.3.2-r3 p1.6, pie-10.1.5) 4.3.2".
-- 
Stefan Richter
-=====-==--= -=== =----
http://arcgraph.de/sr/

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Stefan Richter]

Stefan Richter wrote:
> So, is uninitialized_var() a good fix here?  I'd say it's not a 
> particular good one.  Count the lines of code of dbg_check_old_index() 
> and the maximum indentation level of it.  Then remember the teachings of 
> CodingStyle. :-)  See?  dbg_check_old_index() clearly isn't a prime 
> example of best kernel coding practice.  /Perhaps/ a little bit of 
> refactoring would make it easier to read, and as a bonus side effect, 
> make it unnecessary to use the slightly dangerous and 
> uninitialized_var() macro here.

PS:
On the other hand, it is debug code.  Is it bound to stay in there 
forever?  If not, then it's surely not worth the developer time to 
beautify it.
-- 
Stefan Richter
-=====-==--= -=== =----
http://arcgraph.de/sr/

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Artem Bityutskiy]

On Thu, 2009-07-16 at 13:29 +0200, Stefan Richter wrote:
> my comment was more directed to Subrata rather than you, as a pointer 
> for future changes of this kind.
> 
> By '"Fix compilation warning" is not correct' I mean:
> 
> This subject would be strictly correct if there was a warning message 
> which contained a factual or formal mistake, and the patch would make 
> the warning show up as a factual and formal correct message.
> 
> Yet the patch does nothing else than _suppress a message_.

I agree, so I amended the changelog.

>      3.) Reason:  The compiler wrongly interprets a variable as
>          occasionally unused, even though this is nicely organized,
>          obvious code where the assertion of the compiler is not only
>          wrong, but everybody also can see immediately from the code that
>          there will never be a.
> 
>          Aside from improving the compiler itself to properly handle
>          this, in this case the appropriate action is to use the
>          uninitialized_var() macro --- /unless/ there is a non-intrusive
>          way to refactor the code to be compilable without warning but
>          keep it still nice to read.
> 
>          Why is the latter still better than uninitialized_var()?  Easy:
>          If anybody later changes the code such that he introduces an
>          actual use-without-initialization bug, nobody will notice the
>          bug because it's hidden by the macro.
> 
>          And there are more downsides to the macro:  The readability of
>          code which makes use of it suffers somewhat.  Also, the macro
>          works with gcc (i.e. shuts gcc up), but apparently not with the
>          Intel compiler.
> 
> So, in the majority of types of cases, uninitialized_var() is the wrong 
> course of action.  (Though maybe not in the majority of occurrences of 
> cases.)

I see what you mean. May be in this UBIFS case the code could be
amended indeed. But there is infinite room for nicifications, and I'm
not sure touching this function is worth it. It works and was tested a
lot.

> OK.  Rant over.  It's now time for me to actually have look at the code 
> which is being patched here. :-)  (Sorry, I commented before only on the 
> basis of Subrata's diff and changelog.)
> 
>  From fs/ubifs/commit.c in mainline 2.6.31-rc3:
> 
> Before lower_key, there was obviously already an issue with last_sqnum 
> which received the same treatment as lower_key receives now.  Both of 
> these are provided with a value in a conditional block:
> 
> 	int first = 1;
> 
> 	if (first) {
> 		first = 0;
> 		...
> 	}
> 
> block and first used after this block.  From the looks of the whole 
> function, these two variables indeed don't appear in danger to ever be 
> accessed uninitialized.

Right, this is why I thought this is fake alarm and we can just shyt
gcc up using 'uninitialized_var()'.

> So, is uninitialized_var() a good fix here?  I'd say it's not a 
> particular good one.  Count the lines of code of dbg_check_old_index() 
> and the maximum indentation level of it.  Then remember the teachings of 
> CodingStyle. :-)  See?  dbg_check_old_index() clearly isn't a prime 
> example of best kernel coding practice.  /Perhaps/ a little bit of 
> refactoring would make it easier to read, and as a bonus side effect, 
> make it unnecessary to use the slightly dangerous and 
> uninitialized_var() macro here.

Yes, the function is large and could be split on smaller ones. But I
do not think it is too bad, and still feel like just using
'uninitialized_var()' is just fine. But if someone re-factors the code
I will not object, of course, especially if he also tests it :-)

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Artem Bityutskiy]

On Thu, 2009-07-16 at 13:57 +0200, Stefan Richter wrote:
> Stefan Richter wrote:
> > So, is uninitialized_var() a good fix here?  I'd say it's not a 
> > particular good one.  Count the lines of code of dbg_check_old_index() 
> > and the maximum indentation level of it.  Then remember the teachings of 
> > CodingStyle. :-)  See?  dbg_check_old_index() clearly isn't a prime 
> > example of best kernel coding practice.  /Perhaps/ a little bit of 
> > refactoring would make it easier to read, and as a bonus side effect, 
> > make it unnecessary to use the slightly dangerous and 
> > uninitialized_var() macro here.
> 
> PS:
> On the other hand, it is debug code.  Is it bound to stay in there 
> forever?  If not, then it's surely not worth the developer time to 
> beautify it.

Yes, it is debugging code. It is doing additional consistency/sanity
checks of the internal data structures when you compile it in and enable
it. And yes, I'd like it to stay there forever, because it is a very
nice tool to catch bugs. In fact, I am really keen of this type of
debugging when you have internal checking functions. They help a lot.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

[PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Subrata Modak]

Hey,

>On Thu, 2009-07-16 at 15:11 +0300, Artem Bityutskiy wrote:
>On Thu, 2009-07-16 at 13:57 +0200, Stefan Richter wrote:
> > Stefan Richter wrote:
> > > So, is uninitialized_var() a good fix here?  I'd say it's not a 
> > > particular good one.  Count the lines of code of dbg_check_old_index() 
> > > and the maximum indentation level of it.  Then remember the teachings of 
> > > CodingStyle. :-)  See?  dbg_check_old_index() clearly isn't a prime 
> > > example of best kernel coding practice.  /Perhaps/ a little bit of 
> > > refactoring would make it easier to read, and as a bonus side effect, 
> > > make it unnecessary to use the slightly dangerous and 
> > > uninitialized_var() macro here.
> > 
> > PS:
> > On the other hand, it is debug code.  Is it bound to stay in there 
> > forever?  If not, then it's surely not worth the developer time to 
> > beautify it.
> 
> Yes, it is debugging code. It is doing additional consistency/sanity
> checks of the internal data structures when you compile it in and enable
> it. And yes, I'd like it to stay there forever, because it is a very
> nice tool to catch bugs. In fact, I am really keen of this type of
> debugging when you have internal checking functions. They help a lot.
>

I see from "fs/ubifs/key.h" code that:
	"key_read(ubifs_idx_key(c, idx), &lower_key)"
will definitely initialize 'lower_key'.

Morever,

509 {
510         int lnum, offs, len, err = 0, uninitialized_var(last_level), child_cnt;
511         int first = 1, iip;

'last_level' & 'last_sqnum' definitely gets initialized below:

572                         /* Set last values as though root had a parent */
573                         last_level = le16_to_cpu(idx->level) + 1;
574                         last_sqnum = le64_to_cpu(idx->ch.sqnum) + 1;
575                         key_read(c, ubifs_idx_key(c, idx), &lower_key);

Though it is understandable that GCC (my version 4.4.1) might
not see 'lower_key' assignment directly(hidden through a function call),
it should have predicted 'last_level' & 'last_sqnum'. May be because they
are inside the "if (first) {..." statement.

I understand that there might be no necessity to fix this using
unitialized_var() macro, as, it seems that proper initialization
will take place. However, on debugging, i found something more
interesting. The

key_read() and key_write() functions defined inside "fs/ubifs/key.h"

426 /**
427  * key_read - transform a key to in-memory format.
428  * @c: UBIFS file-system description object
429  * @from: the key to transform
430  * @to: the key to store the result
431  */
432 static inline void key_read(const struct ubifs_info *c, const void *from,
433                             union ubifs_key *to)
434 {
435         const union ubifs_key *f = from;
436 
437         to->u32[0] = le32_to_cpu(f->j32[0]);
438         to->u32[1] = le32_to_cpu(f->j32[1]);
439 }
440 
441 /**
442  * key_write - transform a key from in-memory format.
443  * @c: UBIFS file-system description object
444  * @from: the key to transform
445  * @to: the key to store the result
446  */
447 static inline void key_write(const struct ubifs_info *c,
448                              const union ubifs_key *from, void *to)
449 {
450         union ubifs_key *t = to;
451 
452         t->j32[0] = cpu_to_le32(from->u32[0]);
453         t->j32[1] = cpu_to_le32(from->u32[1]);
454         memset(to + 8, 0, UBIFS_MAX_KEY_LEN - 8);
455 }

does not use:
	"const struct ubifs_info *c"
inside the inline function. I do not see any practical usage of
"const struct ubifs_info *c" in the functions key_read() and key_write().
Is there something which i am missing to understand ?

When i applied the following patch, still the "fs/ubifs/" code compiled fine.
If the below fix is correct, i can try fixing some other functions i saw
having similar defects.
---

diff -uprN b/fs/ubifs/commit.c a/fs/ubifs/commit.c
--- a/fs/ubifs/commit.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/commit.c	2009-07-16 17:42:57.000000000 +0530
@@ -507,7 +507,7 @@ out:
  */
 int dbg_check_old_index(struct ubifs_info *c, struct ubifs_zbranch *zroot)
 {
-	int lnum, offs, len, err = 0, uninitialized_var(last_level), child_cnt;
+	int lnum, offs, len, err = 0, last_level, child_cnt;
 	int first = 1, iip;
 	struct ubifs_debug_info *d = c->dbg;
 	union ubifs_key lower_key, upper_key, l_key, u_key;
@@ -572,7 +572,7 @@ int dbg_check_old_index(struct ubifs_inf
 			/* Set last values as though root had a parent */
 			last_level = le16_to_cpu(idx->level) + 1;
 			last_sqnum = le64_to_cpu(idx->ch.sqnum) + 1;
-			key_read(c, ubifs_idx_key(c, idx), &lower_key);
+			key_read(ubifs_idx_key(c, idx), &lower_key);
 			highest_ino_key(c, &upper_key, INUM_WATERMARK);
 		}
 		key_copy(c, &upper_key, &i->upper_key);
@@ -589,9 +589,9 @@ int dbg_check_old_index(struct ubifs_inf
 			goto out_dump;
 		}
 		/* Check key range */
-		key_read(c, ubifs_idx_key(c, idx), &l_key);
+		key_read(ubifs_idx_key(c, idx), &l_key);
 		br = ubifs_idx_branch(c, idx, child_cnt - 1);
-		key_read(c, &br->key, &u_key);
+		key_read(&br->key, &u_key);
 		if (keys_cmp(c, &lower_key, &l_key) > 0) {
 			err = 5;
 			goto out_dump;
@@ -640,10 +640,10 @@ int dbg_check_old_index(struct ubifs_inf
 		lnum = le32_to_cpu(br->lnum);
 		offs = le32_to_cpu(br->offs);
 		len = le32_to_cpu(br->len);
-		key_read(c, &br->key, &lower_key);
+		key_read(&br->key, &lower_key);
 		if (iip + 1 < le16_to_cpu(idx->child_cnt)) {
 			br = ubifs_idx_branch(c, idx, iip + 1);
-			key_read(c, &br->key, &upper_key);
+			key_read(&br->key, &upper_key);
 		} else
 			key_copy(c, &i->upper_key, &upper_key);
 	}
diff -uprN b/fs/ubifs/debug.c a/fs/ubifs/debug.c
--- a/fs/ubifs/debug.c	2009-07-16 16:18:46.000000000 +0530
+++ b/fs/ubifs/debug.c	2009-07-16 17:32:29.000000000 +0530
@@ -423,7 +423,7 @@ void dbg_dump_node(const struct ubifs_in
 	{
 		const struct ubifs_ino_node *ino = node;
 
-		key_read(c, &ino->key, &key);
+		key_read(&ino->key, &key);
 		printk(KERN_DEBUG "\tkey            %s\n", DBGKEY(&key));
 		printk(KERN_DEBUG "\tcreat_sqnum    %llu\n",
 		       (unsigned long long)le64_to_cpu(ino->creat_sqnum));
@@ -466,7 +466,7 @@ void dbg_dump_node(const struct ubifs_in
 		const struct ubifs_dent_node *dent = node;
 		int nlen = le16_to_cpu(dent->nlen);
 
-		key_read(c, &dent->key, &key);
+		key_read(&dent->key, &key);
 		printk(KERN_DEBUG "\tkey            %s\n", DBGKEY(&key));
 		printk(KERN_DEBUG "\tinum           %llu\n",
 		       (unsigned long long)le64_to_cpu(dent->inum));
@@ -490,7 +490,7 @@ void dbg_dump_node(const struct ubifs_in
 		const struct ubifs_data_node *dn = node;
 		int dlen = le32_to_cpu(ch->len) - UBIFS_DATA_NODE_SZ;
 
-		key_read(c, &dn->key, &key);
+		key_read(&dn->key, &key);
 		printk(KERN_DEBUG "\tkey            %s\n", DBGKEY(&key));
 		printk(KERN_DEBUG "\tsize           %u\n",
 		       le32_to_cpu(dn->size));
@@ -529,7 +529,7 @@ void dbg_dump_node(const struct ubifs_in
 			const struct ubifs_branch *br;
 
 			br = ubifs_idx_branch(c, idx, i);
-			key_read(c, &br->key, &key);
+			key_read(&br->key, &key);
 			printk(KERN_DEBUG "\t%d: LEB %d:%d len %d key %s\n",
 			       i, le32_to_cpu(br->lnum), le32_to_cpu(br->offs),
 			       le32_to_cpu(br->len), DBGKEY(&key));
@@ -998,7 +998,7 @@ int dbg_check_dir_size(struct ubifs_info
 			nlink += 1;
 		kfree(pdent);
 		pdent = dent;
-		key_read(c, &dent->key, &key);
+		key_read(&dent->key, &key);
 	}
 	kfree(pdent);
 
@@ -1066,7 +1066,7 @@ static int dbg_check_key_order(struct ub
 
 	/* Make sure node keys are the same as in zbranch */
 	err = 1;
-	key_read(c, &dent1->key, &key);
+	key_read(&dent1->key, &key);
 	if (keys_cmp(c, &zbr1->key, &key)) {
 		dbg_err("1st entry at %d:%d has key %s", zbr1->lnum,
 			zbr1->offs, DBGKEY(&key));
@@ -1076,7 +1076,7 @@ static int dbg_check_key_order(struct ub
 		goto out_free;
 	}
 
-	key_read(c, &dent2->key, &key);
+	key_read(&dent2->key, &key);
 	if (keys_cmp(c, &zbr2->key, &key)) {
 		dbg_err("2nd entry at %d:%d has key %s", zbr1->lnum,
 			zbr1->offs, DBGKEY(&key));
diff -uprN b/fs/ubifs/dir.c a/fs/ubifs/dir.c
--- a/fs/ubifs/dir.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/dir.c	2009-07-16 17:32:52.000000000 +0530
@@ -439,7 +439,7 @@ static int ubifs_readdir(struct file *fi
 			return 0;
 
 		/* Switch to the next entry */
-		key_read(c, &dent->key, &key);
+		key_read(&dent->key, &key);
 		nm.name = dent->name;
 		dent = ubifs_tnc_next_ent(c, &key, &nm);
 		if (IS_ERR(dent)) {
diff -uprN b/fs/ubifs/gc.c a/fs/ubifs/gc.c
--- a/fs/ubifs/gc.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/gc.c	2009-07-16 17:33:13.000000000 +0530
@@ -546,7 +546,7 @@ int ubifs_garbage_collect_leb(struct ubi
 			int level = le16_to_cpu(idx->level);
 
 			ubifs_assert(snod->type == UBIFS_IDX_NODE);
-			key_read(c, ubifs_idx_key(c, idx), &snod->key);
+			key_read(ubifs_idx_key(c, idx), &snod->key);
 			err = ubifs_dirty_idx_node(c, &snod->key, level, lnum,
 						   snod->offs);
 			if (err)
diff -uprN b/fs/ubifs/journal.c a/fs/ubifs/journal.c
--- a/fs/ubifs/journal.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/journal.c	2009-07-16 17:30:51.000000000 +0530
@@ -583,7 +583,7 @@ int ubifs_jnl_update(struct ubifs_info *
 		xent_key_init(c, &dent_key, dir->i_ino, nm);
 	}
 
-	key_write(c, &dent_key, dent->key);
+	key_write(&dent_key, dent->key);
 	dent->inum = deletion ? 0 : cpu_to_le64(inode->i_ino);
 	dent->type = get_dent_type(inode->i_mode);
 	dent->nlen = cpu_to_le16(nm->len);
@@ -699,7 +699,7 @@ int ubifs_jnl_write_data(struct ubifs_in
 		return -ENOMEM;
 
 	data->ch.node_type = UBIFS_DATA_NODE;
-	key_write(c, key, &data->key);
+	key_write(key, &data->key);
 	data->size = cpu_to_le32(len);
 	zero_data_node_unused(data);
 
@@ -1296,7 +1296,7 @@ int ubifs_jnl_delete_xattr(struct ubifs_
 
 	xent->ch.node_type = UBIFS_XENT_NODE;
 	xent_key_init(c, &xent_key, host->i_ino, nm);
-	key_write(c, &xent_key, xent->key);
+	key_write(&xent_key, xent->key);
 	xent->inum = 0;
 	xent->type = get_dent_type(inode->i_mode);
 	xent->nlen = cpu_to_le16(nm->len);
diff -uprN b/fs/ubifs/key.h a/fs/ubifs/key.h
--- a/fs/ubifs/key.h	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/key.h	2009-07-16 17:37:20.000000000 +0530
@@ -429,8 +429,7 @@ static inline unsigned int key_block_fla
  * @from: the key to transform
  * @to: the key to store the result
  */
-static inline void key_read(const struct ubifs_info *c, const void *from,
-			    union ubifs_key *to)
+static inline void key_read(const void *from, union ubifs_key *to)
 {
 	const union ubifs_key *f = from;
 
@@ -444,8 +443,7 @@ static inline void key_read(const struct
  * @from: the key to transform
  * @to: the key to store the result
  */
-static inline void key_write(const struct ubifs_info *c,
-			     const union ubifs_key *from, void *to)
+static inline void key_write(const union ubifs_key *from, void *to)
 {
 	union ubifs_key *t = to;
 
@@ -461,7 +459,7 @@ static inline void key_write(const struc
  * @to: the key to store the result
  */
 static inline void key_write_idx(const struct ubifs_info *c,
-				 const union ubifs_key *from, void *to)
+                                 const union ubifs_key *from, void *to)
 {
 	union ubifs_key *t = to;
 
diff -uprN b/fs/ubifs/lprops.c a/fs/ubifs/lprops.c
--- a/fs/ubifs/lprops.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/lprops.c	2009-07-16 17:33:30.000000000 +0530
@@ -1142,7 +1142,7 @@ static int scan_check_cb(struct ubifs_in
 		if (snod->type == UBIFS_IDX_NODE) {
 			struct ubifs_idx_node *idx = snod->node;
 
-			key_read(c, ubifs_idx_key(c, idx), &snod->key);
+			key_read(ubifs_idx_key(c, idx), &snod->key);
 			level = le16_to_cpu(idx->level);
 		}
 
diff -uprN b/fs/ubifs/scan.c a/fs/ubifs/scan.c
--- a/fs/ubifs/scan.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/scan.c	2009-07-16 17:33:42.000000000 +0530
@@ -218,7 +218,7 @@ int ubifs_add_snod(const struct ubifs_in
 		 * The key is in the same place in all keyed
 		 * nodes.
 		 */
-		key_read(c, &ino->key, &snod->key);
+		key_read(&ino->key, &snod->key);
 		break;
 	}
 	list_add_tail(&snod->list, &sleb->nodes);
diff -uprN b/fs/ubifs/tnc.c a/fs/ubifs/tnc.c
--- a/fs/ubifs/tnc.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/tnc.c	2009-07-16 17:34:06.000000000 +0530
@@ -510,7 +510,7 @@ static int fallible_read_node(struct ubi
 		struct ubifs_dent_node *dent = node;
 
 		/* All nodes have key in the same place */
-		key_read(c, &dent->key, &node_key);
+		key_read(&dent->key, &node_key);
 		if (keys_cmp(c, key, &node_key) != 0)
 			ret = 0;
 	}
@@ -1713,7 +1713,7 @@ static int validate_data_node(struct ubi
 	}
 
 	/* Make sure the key of the read node is correct */
-	key_read(c, buf + UBIFS_KEY_OFFSET, &key1);
+	key_read(buf + UBIFS_KEY_OFFSET, &key1);
 	if (!keys_eq(c, &zbr->key, &key1)) {
 		ubifs_err("bad key in node at LEB %d:%d",
 			  zbr->lnum, zbr->offs);
@@ -2726,7 +2726,7 @@ int ubifs_tnc_remove_ino(struct ubifs_in
 
 		kfree(pxent);
 		pxent = xent;
-		key_read(c, &xent->key, &key1);
+		key_read(&xent->key, &key1);
 	}
 
 	kfree(pxent);
diff -uprN b/fs/ubifs/tnc_commit.c a/fs/ubifs/tnc_commit.c
--- a/fs/ubifs/tnc_commit.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/tnc_commit.c	2009-07-16 17:34:18.000000000 +0530
@@ -256,7 +256,7 @@ static int layout_leb_in_gaps(struct ubi
 
 		ubifs_assert(snod->type == UBIFS_IDX_NODE);
 		idx = snod->node;
-		key_read(c, ubifs_idx_key(c, idx), &snod->key);
+		key_read(ubifs_idx_key(c, idx), &snod->key);
 		level = le16_to_cpu(idx->level);
 		/* Determine if the index node is in use (not obsolete) */
 		in_use = is_idx_node_in_use(c, &snod->key, level, lnum,
diff -uprN b/fs/ubifs/tnc_misc.c a/fs/ubifs/tnc_misc.c
--- a/fs/ubifs/tnc_misc.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/tnc_misc.c	2009-07-16 17:34:38.000000000 +0530
@@ -305,7 +305,7 @@ static int read_znode(struct ubifs_info 
 		const struct ubifs_branch *br = ubifs_idx_branch(c, idx, i);
 		struct ubifs_zbranch *zbr = &znode->zbranch[i];
 
-		key_read(c, &br->key, &zbr->key);
+		key_read(&br->key, &zbr->key);
 		zbr->lnum = le32_to_cpu(br->lnum);
 		zbr->offs = le32_to_cpu(br->offs);
 		zbr->len  = le32_to_cpu(br->len);
@@ -480,7 +480,7 @@ int ubifs_tnc_read_node(struct ubifs_inf
 	}
 
 	/* Make sure the key of the read node is correct */
-	key_read(c, node + UBIFS_KEY_OFFSET, &key1);
+	key_read(node + UBIFS_KEY_OFFSET, &key1);
 	if (!keys_eq(c, key, &key1)) {
 		ubifs_err("bad key in node at LEB %d:%d",
 			  zbr->lnum, zbr->offs);
diff -uprN b/fs/ubifs/xattr.c a/fs/ubifs/xattr.c
--- a/fs/ubifs/xattr.c	2009-07-16 16:18:47.000000000 +0530
+++ b/fs/ubifs/xattr.c	2009-07-16 17:34:51.000000000 +0530
@@ -468,7 +468,7 @@ ssize_t ubifs_listxattr(struct dentry *d
 
 		kfree(pxent);
 		pxent = xent;
-		key_read(c, &xent->key, &key);
+		key_read(&xent->key, &key);
 	}
 
 	kfree(pxent);

---
Regards--
Subrata

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Artem Bityutskiy]

On Thu, 2009-07-16 at 18:11 +0530, Subrata Modak wrote:
> does not use:
> 	"const struct ubifs_info *c"
> inside the inline function. I do not see any practical usage of
> "const struct ubifs_info *c" in the functions key_read() and key_write().
> Is there something which i am missing to understand ?
> 
> When i applied the following patch, still the "fs/ubifs/" code compiled fine.
> If the below fix is correct, i can try fixing some other functions i saw
> having similar defects.

Yeah, I think the reason why we have this extra argument there is that
we assumed there will be several key schemes. It is possible to add more
than one, but we use only one.

Since you have already spent your time for this, could you please check
if removing this 'c' makes the code smaller? If not, I'd prefer not to
remove it.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c

[Subrata Modak]

On Thu, 2009-07-16 at 15:54 +0300, Artem Bityutskiy wrote:
> On Thu, 2009-07-16 at 18:11 +0530, Subrata Modak wrote:
> > does not use:
> > 	"const struct ubifs_info *c"
> > inside the inline function. I do not see any practical usage of
> > "const struct ubifs_info *c" in the functions key_read() and key_write().
> > Is there something which i am missing to understand ?
> > 
> > When i applied the following patch, still the "fs/ubifs/" code compiled fine.
> > If the below fix is correct, i can try fixing some other functions i saw
> > having similar defects.
> 
> Yeah, I think the reason why we have this extra argument there is that
> we assumed there will be several key schemes. It is possible to add more
> than one, but we use only one.
> 
> Since you have already spent your time for this, could you please check
> if removing this 'c' makes the code smaller? If not, I'd prefer not to

Ok. I would let you know soon.

Regards--
Subrata

> remove it.
>