Re: [PATCH] jffs2: remove unneeded conditions

[Brian Norris <computersforpeace@gmail.com>]

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