Re: [f2fs-dev] [PATCH] fsck.f2fs: correct return value

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 08/05, Chao Yu wrote:
> As Norbert Lange reported:
> 
> "
> $ fsck.f2fs -a /dev/mmcblk0p5; echo $?
> Info: Fix the reported corruption.
> Info: Mounted device!
> Info: Check FS only on RO mounted device
> Error: Failed to open the device!
> 255
> "
> 
> Michael Laß reminds:
> 
> "
> I think the return value is exactly the problem here. See fsck(8) (
> https://linux.die.net/man/8/fsck) which specifies the return values.
> Systemd looks at these and decides how to proceed:
> 
> https://github.com/systemd/systemd/blob/a859abf062cef1511e4879c4ee39c6036ebeaec8/src/fsck/fsck.c#L407
> 
> That means, if fsck.f2fs returns 255, then
> the FSCK_SYSTEM_SHOULD_REBOOT bit is set and systemd will reboot.
> "
> 
> So the problem here is fsck.f2fs didn't return correct value to userspace
> apps, result in later unexpected behavior of rebooting, let's fix this.
> 
> Reported-by: Norbert Lange <nolange79@gmail.com>
> Reported-by: Michael Laß <bevan@bi-co.net>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fsck/fsck.h | 11 +++++++++++
>  fsck/main.c | 27 +++++++++++++++++++++------
>  2 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/fsck/fsck.h b/fsck/fsck.h
> index e86730c34fc4..c5e85fefa07b 100644
> --- a/fsck/fsck.h
> +++ b/fsck/fsck.h
> @@ -13,6 +13,17 @@
>  
>  #include "f2fs.h"
>  
> +enum {
> +	FSCK_SUCCESS                 = 0,
> +	FSCK_ERROR_CORRECTED         = 1 << 0,
> +	FSCK_SYSTEM_SHOULD_REBOOT    = 1 << 1,
> +	FSCK_ERRORS_LEFT_UNCORRECTED = 1 << 2,
> +	FSCK_OPERATIONAL_ERROR       = 1 << 3,
> +	FSCK_USAGE_OR_SYNTAX_ERROR   = 1 << 4,
> +	FSCK_USER_CANCELLED          = 1 << 5,
> +	FSCK_SHARED_LIB_ERROR        = 1 << 7,
> +};
> +
>  struct quota_ctx;
>  
>  #define FSCK_UNMATCHED_EXTENT		0x00000001
> diff --git a/fsck/main.c b/fsck/main.c
> index 9a1596f35e79..11d472b9941c 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -630,7 +630,7 @@ void f2fs_parse_options(int argc, char *argv[])
>  	error_out(prog);
>  }
>  
> -static void do_fsck(struct f2fs_sb_info *sbi)
> +static int do_fsck(struct f2fs_sb_info *sbi)
>  {
>  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
>  	u32 flag = le32_to_cpu(ckpt->ckpt_flags);
> @@ -655,7 +655,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>  			} else {
>  				MSG(0, "[FSCK] F2FS metadata   [Ok..]");
>  				fsck_free(sbi);
> -				return;
> +				return FSCK_SUCCESS;
>  			}
>  
>  			if (!c.ro)
> @@ -687,7 +687,7 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>  		ret = quota_init_context(sbi);
>  		if (ret) {
>  			ASSERT_MSG("quota_init_context failure: %d", ret);
> -			return;
> +			return FSCK_OPERATIONAL_ERROR;
>  		}
>  	}
>  	fsck_chk_orphan_node(sbi);
> @@ -695,8 +695,14 @@ static void do_fsck(struct f2fs_sb_info *sbi)
>  			F2FS_FT_DIR, TYPE_INODE, &blk_cnt, NULL);
>  	fsck_chk_quota_files(sbi);
>  
> -	fsck_verify(sbi);
> +	ret = fsck_verify(sbi);
>  	fsck_free(sbi);
> +
> +	if (!c.bug_on)
> +		return FSCK_SUCCESS;
> +	if (!ret)
> +		return FSCK_ERROR_CORRECTED;

I applied this to get FSCK_ERROR_CORRECTED.

--- a/fsck/fsck.c
+++ b/fsck/fsck.c
@@ -3165,6 +3165,8 @@ int fsck_verify(struct f2fs_sb_info *sbi)
                        is_set_ckpt_flags(cp, CP_QUOTA_NEED_FSCK_FLAG)) {
                        write_checkpoints(sbi);
                }
+               /* to return FSCK_ERROR_CORRECTED */
+               ret = 0;
        }
        return ret;
 }

> +	return FSCK_ERRORS_LEFT_UNCORRECTED;
>  }
>  
>  static void do_dump(struct f2fs_sb_info *sbi)
> @@ -833,11 +839,15 @@ int main(int argc, char **argv)
>  	if (c.func != DUMP && f2fs_devs_are_umounted() < 0) {
>  		if (errno == EBUSY) {
>  			ret = -1;
> +			if (c.func == FSCK)
> +				ret = FSCK_OPERATIONAL_ERROR;
>  			goto quick_err;
>  		}
>  		if (!c.ro || c.func == DEFRAG) {
>  			MSG(0, "\tError: Not available on mounted device!\n");
>  			ret = -1;
> +			if (c.func == FSCK)
> +				ret = FSCK_OPERATIONAL_ERROR;
>  			goto quick_err;
>  		}
>  
> @@ -854,6 +864,8 @@ int main(int argc, char **argv)
>  	/* Get device */
>  	if (f2fs_get_device_info() < 0) {
>  		ret = -1;
> +		if (c.func == FSCK)
> +			ret = FSCK_OPERATIONAL_ERROR;
>  		goto quick_err;
>  	}
>  
> @@ -873,7 +885,7 @@ fsck_again:
>  
>  	switch (c.func) {
>  	case FSCK:
> -		do_fsck(sbi);
> +		ret = do_fsck(sbi);
>  		break;
>  #ifdef WITH_DUMP
>  	case DUMP:
> @@ -935,8 +947,11 @@ retry:
>  		}
>  	}
>  	ret = f2fs_finalize_device();
> -	if (ret < 0)
> +	if (ret) {
> +		if (c.func == FSCK)
> +			return FSCK_OPERATIONAL_ERROR;
>  		return ret;
> +	}
>  
>  	printf("\nDone: %lf secs\n", (get_boottime_ns() - start) / 1000000000.0);
>  	return 0;
> -- 
> 2.26.2


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel