[bug report] fs/ntfs3: Add initialization of super block

[bug report] fs/ntfs3: Add initialization of super block

[Dan Carpenter]

Hello Konstantin Komarov,

The patch 82cae269cfa9: "fs/ntfs3: Add initialization of super block"
from Aug 13, 2021, leads to the following
Smatch static checker warning:

	fs/ntfs3/index.c:238 bmp_buf_get()
	warn: 'bh' could be an error pointer

fs/ntfs3/index.c
    229 	data_size = le64_to_cpu(b->nres.data_size);
    230 	if (WARN_ON(off >= data_size)) {
    231 		/* looks like filesystem error */
    232 		return -EINVAL;
    233 	}
    234 
    235 	valid_size = le64_to_cpu(b->nres.valid_size);
    236 
    237 	bh = ntfs_bread_run(sbi, &indx->bitmap_run, off);
--> 238 	if (!bh)
    239 		return -EIO;
    240 
    241 	if (IS_ERR(bh))

This is not a bug, but it is wrong style.  When a function returns both
error pointers and NULL then the NULL return is means the feature is
disabled.  It's not an error.  Just that the feature is turned off
deliberately in the Kconfig or whatever.  Don't print an error message,
just continue with the feature disabled as the admin has requested.

But here NULL is just an error.  The ntfs_bread_run() should do:

	bh = ntfs_bread();
	if (!bh)
		return ERR_PTR(-EIO);
	return bh;

    242 		return PTR_ERR(bh);
    243 
    244 	bbuf->bh = bh;
    245 

regards,
dan carpenter

Re: [bug report] fs/ntfs3: Add initialization of super block

[Kari Argillander]

On Tue, Aug 24, 2021 at 10:58:19AM +0300, Dan Carpenter wrote:
> Hello Konstantin Komarov,
> 
> The patch 82cae269cfa9: "fs/ntfs3: Add initialization of super block"
> from Aug 13, 2021, leads to the following
> Smatch static checker warning:
> 
> 	fs/ntfs3/index.c:238 bmp_buf_get()
> 	warn: 'bh' could be an error pointer
> 
> fs/ntfs3/index.c
>     229 	data_size = le64_to_cpu(b->nres.data_size);
>     230 	if (WARN_ON(off >= data_size)) {
>     231 		/* looks like filesystem error */
>     232 		return -EINVAL;
>     233 	}
>     234 
>     235 	valid_size = le64_to_cpu(b->nres.valid_size);
>     236 
>     237 	bh = ntfs_bread_run(sbi, &indx->bitmap_run, off);
> --> 238 	if (!bh)
>     239 		return -EIO;
>     240 
>     241 	if (IS_ERR(bh))
> 
> This is not a bug, but it is wrong style.  When a function returns both
> error pointers and NULL then the NULL return is means the feature is
> disabled.  It's not an error.  Just that the feature is turned off
> deliberately in the Kconfig or whatever.  Don't print an error message,
> just continue with the feature disabled as the admin has requested.
> 
> But here NULL is just an error.  The ntfs_bread_run() should do:

Agreed.

> 
> 	bh = ntfs_bread();
> 	if (!bh)
> 		return ERR_PTR(-EIO);
> 	return bh;

Can also be like below but probably your version is more readable.

return ntfs_bread(sb, lbo >> sb->s_blocksize_bits) ? : ERR_PTR(-EIO);

> 
>     242 		return PTR_ERR(bh);
>     243 
>     244 	bbuf->bh = bh;
>     245 
> 
> regards,
> dan carpenter