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

[Norbert Lange <nolange79@gmail.com>]

Am Mi., 5. Aug. 2020 um 10:10 Uhr schrieb Chao Yu <yuchao0@huawei.com>:
>
> 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;
> +       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);

This returncode will never be used.

>                 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
>

Norbert


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