> On Aug 10, 2015, at 3:34 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> My static checker complains because it is not necessary to test
> "if (*bh)" when we just tested "if (!*bh)" on the lines before.
> We can remove the condition and pull everything in one indent level.
>
> Then we have another "if (unlikely(!*bh))" check and the only way that
> can be true is if the "if (!buffer_uptodate(*bh))" condition was true so
> we can combine them together.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 8313ca3..736f12e 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -80,18 +80,15 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,
> if (!*bh)
> *bh = sb_getblk(sb, mmp_block);
> if (!*bh)
> return -ENOMEM;
It may also be clearer if the second "if (!*bh)" check were below
the first one, like:
if (!*bh) {
*bh = sb_getblk(sb, mmp_block);
if (!*bh)
return -ENOMEM;
}
That moves the second check out-of-line since it can only happen if
the first one is already true.
That said, it looks like this second "if (!*bh)" check was added
later on in commit 860d21e2 and the original logic made sense when
it was written, so that the ext4_warning() message below would get
printed in this case. Otherwise the mount can fail without any
message being printed on the console at all. Other ext4 mount
failures print error messages, so this should be done here also.
> - if (*bh) {
> - get_bh(*bh);
> - lock_buffer(*bh);
> - (*bh)->b_end_io = end_buffer_read_sync;
> - submit_bh(READ_SYNC | REQ_META | REQ_PRIO, *bh);
> - wait_on_buffer(*bh);
> - if (!buffer_uptodate(*bh)) {
> - brelse(*bh);
> - *bh = NULL;
> - }
> - }
> - if (unlikely(!*bh)) {
> +
> + get_bh(*bh);
> + lock_buffer(*bh);
> + (*bh)->b_end_io = end_buffer_read_sync;
> + submit_bh(READ_SYNC | REQ_META | REQ_PRIO, *bh);
> + wait_on_buffer(*bh);
> + if (!buffer_uptodate(*bh)) {
> + brelse(*bh);
> + *bh = NULL;
> ext4_warning(sb, "Error while reading MMP block %llu",
> mmp_block);
> return -EIO;
Maybe it would be better to move the ext4_warning() to the end of the
function and then set rc in the failure cases with a goto err: and
print the message only once?
Cheers, Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
