[PATCH 02/06] Fix compilation warning for fs/ubifs/commit.c
From: Subrata Modak
Date: Thu Jul 16 2009 - 08:42:13 EST
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/