From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pd0-x22d.google.com ([2607:f8b0:400e:c02::22d]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZCZKh-0004PR-4I for linux-mtd@lists.infradead.org; Tue, 07 Jul 2015 20:19:11 +0000 Received: by pdbep18 with SMTP id ep18so131556055pdb.1 for ; Tue, 07 Jul 2015 13:18:50 -0700 (PDT) Date: Tue, 7 Jul 2015 13:18:43 -0700 From: Brian Norris To: Wei Fang Cc: dwmw2@infradead.org, linux-mtd@lists.infradead.org Subject: Re: [PATCH] jffs2: remove unneeded conditions Message-ID: <20150707201843.GH18370@brian-ubuntu> References: <558E59C9.2070207@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <558E59C9.2070207@huawei.com> List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 > --- > 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