Re: [PATCH v2 1/4] overlay: correct fsck.overlay exit code

["zhangyi (F)" <yi.zhang@xxxxxxxxxx>]

On 2018/10/16 17:45, Amir Goldstein Wrote:
> On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote:
>>
>> fsck.overlay should return correct exit code to show the file system
>> status after fsck, instead of return 0 means consistency and !0 means
>> inconsistency or something bad happened.
>>
>> Fix the following three exit code after running fsck.overlay:
>>
>> - Return FSCK_OK if the input file system is consistent,
>> - Return FSCK_NONDESTRUCT if the file system inconsistent errors
>>   corrected,
>> - Return FSCK_UNCORRECTED if the file system still have inconsistent
>>   errors.
>>
>> This patch also add a helper function to run fsck.overlay and check
>> the return value is expected or not.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@xxxxxxxxxx>
>> ---
>>  common/overlay    | 29 +++++++++++++++++++++++++++++
>>  tests/overlay/045 | 27 +++++++++------------------
>>  tests/overlay/046 | 36 ++++++++++++------------------------
>>  tests/overlay/056 |  9 +++------
>>  4 files changed, 53 insertions(+), 48 deletions(-)
>>
>> diff --git a/common/overlay b/common/overlay
>> index b526f24..4cc2829 100644
>> --- a/common/overlay
>> +++ b/common/overlay
>> @@ -12,6 +12,16 @@ export OVL_XATTR_NLINK="trusted.overlay.nlink"
>>  export OVL_XATTR_UPPER="trusted.overlay.upper"
>>  export OVL_XATTR_METACOPY="trusted.overlay.metacopy"
>>
>> +# Export exit code used by fsck.overlay program
>> +export FSCK_OK=0
>> +export FSCK_NONDESTRUCT=1
>> +export FSCK_REBOOT=2
>> +export FSCK_UNCORRECTED=4
>> +export FSCK_ERROR=8
>> +export FSCK_USAGE=16
>> +export FSCK_CANCELED=32
>> +export FSCK_LIBRARY=128
>> +
>>  # helper function to do the actual overlayfs mount operation
>>  _overlay_mount_dirs()
>>  {
>> @@ -193,6 +203,25 @@ _overlay_fsck_dirs()
>>                            -o workdir=$workdir $*
>>  }
>>
>> +# Run fsck and check the return value
>> +_run_check_fsck()
>> +{
>> +       # The first arguments is the expected fsck program exit code, the
>> +       # remaining arguments are the input parameters of the fsck program.
>> +       local expect_ret=$1
>> +       local lowerdir=$2
>> +       local upperdir=$3
>> +       local workdir=$4
>> +       shift 4
>> +
>> +       _overlay_fsck_dirs $lowerdir $upperdir $workdir $* >> \
>> +                       $seqres.full 2>&1
>> +       fsck_ret=$?
>> +
>> +       [[ "$fsck_ret" == "$expect_ret" ]] || \
>> +               echo "fsck return unexpected value:$expect_ret,$fsck_ret"
>> +}
> 
> Looks good.
> 
> Please consider the name _overlay_repair_dirs() with reference to
> _repair_scratch_fs(), which is used for roughly the same purpose.
> 

_run_check_fsck() is helper used to test fsck and may expect to return
"error" exit code when we do exception tests(see patch 4), but
_repair_scratch_fs() always want to return "correct" exit code.

> BTW, the tests generic/330 generic/332 when run with -overlay
> over xfs with reflink support seem to call _repair_scratch_fs() which does:
>         # Let's hope fsck -y suffices...
>         fsck -t $FSTYP -y $SCRATCH_DEV 2>&1
>         local res=$?
>         case $res in
>         0|1|2)
>                 res=0
>                 ;;
>         *)
>                 _dump_err2 "fsck.$FSTYP failed, err=$res"
> 
> How come fsck.overlay is not complaining about missing arguments??
> 
> The rest of the generic tests that call _repair_scratch_fs() have
> _require_dm_target() which _require_block_device(), so don't run with overlay.
> 
> If you do sort this out and add overlay support to
> _repair_scratch_fs() its probably
> worth replacing 0|1|2) with FSCK_ constants.
> 

Thanks for pointing this out, I think we do something like below can fix
this problem, what do you think?

diff --git a/common/overlay b/common/overlay
index 2896594..b9fe1ee 100644
--- a/common/overlay
+++ b/common/overlay
@@ -226,6 +226,14 @@ _run_check_fsck()
                echo "fsck return unexpected value:$expect_ret,$fsck_ret"
 }

+_scratch_overlay_repair()
+{
+       _overlay_fsck_dirs $OVL_BASE_SCRATCH_MNT/$OVL_LOWER \
+                          $OVL_BASE_SCRATCH_MNT/$OVL_UPPER \
+                          $OVL_BASE_SCRATCH_MNT/$OVL_WORK \
+                          -y
+}
+
 _overlay_check_dirs()
 {
        local lowerdir=$1
diff --git a/common/rc b/common/rc
index 38d9de7..7127539 100644
--- a/common/rc
+++ b/common/rc
@@ -1100,6 +1100,18 @@ _repair_scratch_fs()
        fi
        return $res
         ;;
+    overlay)
+       _scratch_overlay_repair 2>&1
+       local res=$?
+       case $res in
+       $FSCK_OK|$FSCK_NONDESTRUCT)
+               res=0
+               ;;
+       *)
+               _dump_err2 "fsck.overlay failed, err=$res"
+       esac
+       return $res
+       ;;
     *)
         # Let's hope fsck -y suffices...
         fsck -t $FSTYP -y $SCRATCH_DEV 2>&1

Thanks,
Yi.