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

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

---
Regards--
Subrata

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

Commit Message

Comments

Patch