[PATCH] jffs2: remove unneeded conditions
Brian Norris
computersforpeace at gmail.com
Tue Jul 7 13:18:43 PDT 2015
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 at 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
More information about the linux-mtd
mailing list