| Message ID | 558E59C9.2070207@huawei.com |
|---|---|
| State | Rejected |
| Headers | show |
Hi Wei, On Sat, Jun 27, 2015 at 04:07:37PM +0800, Wei Fang wrote: > 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? > Signed-off-by: Wei Fang <fangwei1@huawei.com> > --- > fs/jffs2/readinode.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c > index dddbde4..b9bd3ad 100644 > --- a/fs/jffs2/readinode.c > +++ b/fs/jffs2/readinode.c > @@ -1059,8 +1059,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf > > case 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. > - 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. > 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_inf > > case 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_inf > break; > > 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. On 7/8/2015 4:18 AM, Brian Norris wrote: > Hi Wei, > > On Sat, Jun 27, 2015 at 04:07:37PM +0800, Wei Fang wrote: >> 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? > >> Signed-off-by: Wei Fang <fangwei1@huawei.com> >> --- >> fs/jffs2/readinode.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c >> index dddbde4..b9bd3ad 100644 >> --- a/fs/jffs2/readinode.c >> +++ b/fs/jffs2/readinode.c >> @@ -1059,8 +1059,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf >> >> case 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. 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 > >> - 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. > >> 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_inf >> >> case 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_inf >> break; >> >> 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, On 2015/7/8 9:45, Sheng Yong wrote: > On 7/8/2015 4:18 AM, Brian Norris wrote: >> 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! >> Also, did you test these changes? Are you solving any real problem? >> >>> Signed-off-by: Wei Fang <fangwei1@huawei.com> >>> --- >>> fs/jffs2/readinode.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c >>> index dddbde4..b9bd3ad 100644 >>> --- a/fs/jffs2/readinode.c >>> +++ b/fs/jffs2/readinode.c >>> @@ -1059,8 +1059,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf >>> >>> case 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. > 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. 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. > > And as Brain said, the modification needs some test. > > thanks, > Sheng >> >>> - 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. >> >>> 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_inf >>> >>> case 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_inf >>> break; >>> >>> 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 >> >> Thanks, >> Brian >> >> ______________________________________________________ >> Linux MTD discussion mailing list >> http://lists.infradead.org/mailman/listinfo/linux-mtd/ >> >>
diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c index dddbde4..b9bd3ad 100644 --- a/fs/jffs2/readinode.c +++ b/fs/jffs2/readinode.c @@ -1059,8 +1059,7 @@ static int jffs2_get_inode_nodes(struct jffs2_sb_info *c, struct jffs2_inode_inf case 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_inf case 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_inf break; 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;
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. Signed-off-by: Wei Fang <fangwei1@huawei.com> --- fs/jffs2/readinode.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)