jffs2: remove unneeded conditions
PATCH 1/1 jffs2: remove unneeded conditions
Since len must not be smaller than JFFS2_MIN_NODE_HEADER, if "len < X" is true, than "JFFS2_MIN_NODE_HEADER < X" must be true, so it can be removed.
--- fs/jffs2/readinode.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
fs/jffs2/readinode.c +3 -6
@@ -1059,8 +1059,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infcase JFFS2_NODETYPE_DIRENT : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_raw_dirent ) && - len <sizeof (struct jffs2_raw_dirent )) { +if (len <sizeof (struct jffs2_raw_dirent )) { err =read_more (c, ref,sizeof (struct jffs2_raw_dirent ), &len, buf);if (unlikely (err)) goto free_out;
@@ -1074,8 +1073,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infcase JFFS2_NODETYPE_INODE : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_raw_inode ) && - len <sizeof (struct jffs2_raw_inode )) { +if (len <sizeof (struct jffs2_raw_inode )) { err =read_more (c, ref,sizeof (struct jffs2_raw_inode ), &len, buf);if (unlikely (err)) goto free_out;
@@ -1088,8 +1086,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infbreak ;default : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_unknown_node ) && - len <sizeof (struct jffs2_unknown_node )) { +if (len <sizeof (struct jffs2_unknown_node )) { err =read_more (c, ref,sizeof (struct jffs2_unknown_node ), &len, buf);if (unlikely (err)) goto free_out;
Signature · 1 line
1.8.3.1
Hi Wei,
Quoted 3 lines · Wei Fang
Since len must not be smaller than
JFFS2_MIN_NODE_HEADER, if "len < X" is true, than "JFFS2_MIN_NODE_HEADER< X" must be true, so it can be removed.
Huh? This comment doesn't exactly make sense to me. It seems like when reasoning about a safety check, you're assuming the safety check will already pass. Can you elaborate your reasoning here?
Also, did you test these changes? Are you solving any real problem?
Quoted 10 lines
Signed-off-by Wei Fang <[email protected]>--- fs/jffs2/readinode.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)fs/jffs2/readinode.c +0
-0
fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1062
Quoted 4 lines
fs/jffs2/readinode.c +0
-1@@ -1059,8 +1059,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infcase JFFS2_NODETYPE_DIRENT : - if (JFFS2_MIN_NODE_HEADER < sizeof(struct jffs2_raw_dirent) &&
^^ The original comparison here is kind of strange. I see:
#define JFFS2_MIN_NODE_HEADER sizeof(struct jffs2_raw_dirent)
which means that we're comparing:
if (sizeof(struct jffs2_raw_dirent) < sizeof(struct jffs2_raw_dirent) && ...)
AFAIK, that comparison will *always* be false, and so the entire condition will always be false. Not sure if that's intentional.
fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1062
Quoted 2 lines
fs/jffs2/readinode.c +1
-1- len < sizeof(struct jffs2_raw_dirent)) { +if (len <sizeof (struct jffs2_raw_dirent )) {
Therefore, the "refactoring" you are doing seems to actually make a logical change. If nothing else, it makes it harder (likely impossible) for the compiler to reason that the conditional code is all dead code. I'm not sure if that's a good or a bad thing, as I haven't figured out the full intent of this code in the first place.
Quoted 14 lines
fs/jffs2/readinode.c +1
-2err =read_more (c, ref,sizeof (struct jffs2_raw_dirent ), &len, buf);if (unlikely (err)) goto free_out;@@ -1074,8 +1073,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infcase JFFS2_NODETYPE_INODE : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_raw_inode ) && - len <sizeof (struct jffs2_raw_inode )) { +if (len <sizeof (struct jffs2_raw_inode )) { err =read_more (c, ref,sizeof (struct jffs2_raw_inode ), &len, buf);if (unlikely (err)) goto free_out;
fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1092
Quoted 9 lines
fs/jffs2/readinode.c +1
-2@@ -1088,8 +1086,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infbreak ;default : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_unknown_node ) && - len <sizeof (struct jffs2_unknown_node )) { +if (len <sizeof (struct jffs2_unknown_node )) { err =read_more (c, ref,sizeof (struct jffs2_unknown_node ), &len, buf);if (unlikely (err)) goto free_out;
At any rate, I'm not confident in this patch without a lot more explanation, so I will not be taking it as-is.
Thanks,
Brian
Hi, Brain and Wei Fang,
#CCed Artem Bityutskiy.
Quoted 30 lines · Brian Norris
Hi Wei,
Quoted 3 lines · Wei Fang
Since len must not be smaller than
JFFS2_MIN_NODE_HEADER, if "len < X" is true, than "JFFS2_MIN_NODE_HEADER< X" must be true, so it can be removed.Huh? This comment doesn't exactly make sense to me. It seems like when reasoning about a safety check, you're assuming the safety check will already pass. Can you elaborate your reasoning here?
Also, did you test these changes? Are you solving any real problem?
Quoted 10 lines
Signed-off-by Wei Fang <[email protected]>--- fs/jffs2/readinode.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)fs/jffs2/readinode.c +0
-0fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1062
Quoted 4 lines
fs/jffs2/readinode.c +0
-1@@ -1059,8 +1059,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infcase JFFS2_NODETYPE_DIRENT : - if (JFFS2_MIN_NODE_HEADER < sizeof(struct jffs2_raw_dirent) &&^^ The original comparison here is kind of strange. I see:
Quoted 8 lines
#define
JFFS2_MIN_NODE_HEADERsizeof(struct jffs2_raw_dirent)which means that we're comparing:
if (sizeof(struct jffs2_raw_dirent) < sizeof(struct jffs2_raw_dirent) && ...)AFAIK, that comparison will *always* be false, and so the entire condition will always be false. Not sure if that's intentional.
According to the comment,
"At this point we don't know the type of the node we're going
to read, so we do not know the size of its header. In order to minimize the amount of flash IO we assume the node has size = JFFS2_MIN_NODE_HEADER."
in order to save overhead of flash IO, jffs2 reads JFFS2_MIN_NODE_HEADER bytes first. This is enough to detect the node type. IMO, for node whose type is JFFS2_NODETYPE_DIRENT, there is no need to read more, so the whole block of if statement can be removed.
And as Brain said, the modification needs some test.
thanks,
Sheng
Quoted 45 lines
fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1062
Quoted 3 lines
fs/jffs2/readinode.c +1
-1- len < sizeof(struct jffs2_raw_dirent)) { +if (len <sizeof (struct jffs2_raw_dirent )) {Therefore, the "refactoring" you are doing seems to actually make a logical change. If nothing else, it makes it harder (likely impossible) for the compiler to reason that the conditional code is all dead code. I'm not sure if that's a good or a bad thing, as I haven't figured out the full intent of this code in the first place.
Quoted 14 lines
fs/jffs2/readinode.c +1
-2err =read_more (c, ref,sizeof (struct jffs2_raw_dirent ), &len, buf);if (unlikely (err)) goto free_out;@@ -1074,8 +1073,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infcase JFFS2_NODETYPE_INODE : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_raw_inode ) && - len <sizeof (struct jffs2_raw_inode )) { +if (len <sizeof (struct jffs2_raw_inode )) { err =read_more (c, ref,sizeof (struct jffs2_raw_inode ), &len, buf);if (unlikely (err)) goto free_out;fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1092
Quoted 9 lines
fs/jffs2/readinode.c +1
-2@@ -1088,8 +1086,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infbreak ;default : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_unknown_node ) && - len <sizeof (struct jffs2_unknown_node )) { +if (len <sizeof (struct jffs2_unknown_node )) { err =read_more (c, ref,sizeof (struct jffs2_unknown_node ), &len, buf);if (unlikely (err)) goto free_out;At any rate, I'm not confident in this patch without a lot more explanation, so I will not be taking it as-is.
Thanks,
Brian______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Hi Brian and Sheng,
Quoted 4 lines · Sheng Yong
Quoted 3 lines · Brian Norris
Huh? This comment doesn't exactly make sense to me. It seems like when reasoning about a safety check, you're assuming the safety check will already pass. Can you elaborate your reasoning here?
Hmm, I think my poor comment confused you:(.
As we known, len >= JFFS2_MIN_NODE_HEADER, and depending on that point, my reasoning is like this:
len < X JFFS2_MIN_NODE_HEADER < X JFFS2_MIN_NODE_HEADER < X && len < X
if true must be true true
if false don't care false
so the final result can be decided by "len < X", and I thought "JFFS2_MIN_NODE_HEADER < X" is unneeded.
Do I miss something?
Yes. Thanks to you, I realized that "JFFS2_MIN_NODE_HEADER < X" can be judged by compiler, and if false, the scope of if would be ignored by compiler. So this comparison is very useful!
Quoted 27 lines
Quoted 19 lines
Also, did you test these changes? Are you solving any real problem?
Quoted 10 lines
Signed-off-by Wei Fang <[email protected]>--- fs/jffs2/readinode.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)fs/jffs2/readinode.c +0
-0fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1062
Quoted 4 lines
fs/jffs2/readinode.c +0
-1@@ -1059,8 +1059,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infcase JFFS2_NODETYPE_DIRENT : - if (JFFS2_MIN_NODE_HEADER < sizeof(struct jffs2_raw_dirent) &&^^ The original comparison here is kind of strange. I see:
Quoted 8 lines
#define
JFFS2_MIN_NODE_HEADERsizeof(struct jffs2_raw_dirent)which means that we're comparing:
if (sizeof(struct jffs2_raw_dirent) < sizeof(struct jffs2_raw_dirent) && ...)AFAIK, that comparison will *always* be false, and so the entire condition will always be false. Not sure if that's intentional.
Quoted 9 lines
According to the comment,
"At this point we don't know the type of the node we're going
to read, so we do not know the size of its header. In order to minimize the amount of flash IO we assume the node has size =JFFS2_MIN_NODE_HEADER."
in order to save overhead of flash IO, jffs2 readsJFFS2_MIN_NODE_HEADERbytes first. This is enough to detect the node type. IMO, for node whose type isJFFS2_NODETYPE_DIRENT, there is no need to read more, so the whole block of if statement can be removed.
When we do some changes to the node type structures, JFFS2_MIN_NODE_HEADER may be redefined, and the result of this comparison will be changed. Though jffs2 is very stable, and changes to structures on flash won't happen, I still think remaining this kind of comparison can make the code more readable and maintainable.
Whatever, as I said, this scope of if can be ignored by compiler, removing it doesn't make any sense.
Quoted 41 lines
And as Brain said, the modification needs some test.
thanks,
ShengQuoted 36 lines
fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1062
Quoted 3 lines
fs/jffs2/readinode.c +1
-1- len < sizeof(struct jffs2_raw_dirent)) { +if (len <sizeof (struct jffs2_raw_dirent )) {Therefore, the "refactoring" you are doing seems to actually make a logical change. If nothing else, it makes it harder (likely impossible) for the compiler to reason that the conditional code is all dead code. I'm not sure if that's a good or a bad thing, as I haven't figured out the full intent of this code in the first place.
Quoted 14 lines
fs/jffs2/readinode.c +1
-2err =read_more (c, ref,sizeof (struct jffs2_raw_dirent ), &len, buf);if (unlikely (err)) goto free_out;@@ -1074,8 +1073,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infcase JFFS2_NODETYPE_INODE : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_raw_inode ) && - len <sizeof (struct jffs2_raw_inode )) { +if (len <sizeof (struct jffs2_raw_inode )) { err =read_more (c, ref,sizeof (struct jffs2_raw_inode ), &len, buf);if (unlikely (err)) goto free_out;fs/jffs2/readinode.c static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf · line 1092
Quoted 9 lines
fs/jffs2/readinode.c +1
-2@@ -1088,8 +1086,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_infbreak ;default : -if (JFFS2_MIN_NODE_HEADER <sizeof (struct jffs2_unknown_node ) && - len <sizeof (struct jffs2_unknown_node )) { +if (len <sizeof (struct jffs2_unknown_node )) { err =read_more (c, ref,sizeof (struct jffs2_unknown_node ), &len, buf);if (unlikely (err)) goto free_out;At any rate, I'm not confident in this patch without a lot more explanation, so I will not be taking it as-is.
Please ignore this patch. Adding comment to "JFFS2_MIN_NODE_HEADER < X" may be useful:).
Thanks,
Wei
Quoted 9 lines
Quoted 9 lines
Thanks,
Brian______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/