Re: [PATCH v2 2/4] overlay: fix exit code for some fsck.overlay valid cases

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Oct 18, 2018 at 6:43 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
>
> On 2018/10/16 17:26, Amir Goldstein Wrote:
> > On Tue, Oct 16, 2018 at 10:32 AM zhangyi (F) <yi.zhang@huawei.com> wrote:
> >>
> >> Some valid test cases about fsck.overlay may be not valid enough now,
> >> they lose the impure xattr on the parent directory of the simluated
> >> redirect directory, and lose the whiteout which use to cover the origin
> >> lower object. Then fsck.overlay will fix these two inconsistency which
> >> are not those test cases want to cover, thus it will lead to
> >> fsck.overlay return FSCK_NONDESTRUCT instead of FSCK_OK. Fix these by
> >> complement the missing overlay related features.
> >>
> >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> >> ---
> >
> > Ok. I think it's fine if we merge this fix now, but this way it is going
> > to be quite hard to maintain this test.
> >
> > Imagine every time that you add another feature to fsck.overlay,
> > say "add overlay features xattr", fsck will start returning FSCK_NONDESTRUCT
> > and break this test.
> >
> > Perhaps it would have been better to construct the test cases by:
> > - mount overlay
> > - create some copied up/ redirected  dirs and whiteouts
> > - umount overlay
> > - make minor modifications to upper/lower layer
> > - run fsck
> >
> > Then you wouldn't need to worry about things like impure parent dir
> > and future overlay features.
> >
> > I will leave it to you to decide if you want to fix this now or the
> > next time around...
> >
>
> Indeed, I thought about this choice. If we create overlay on-disk features
> (xattrs，whiteouts...) through overlayfs, the fsck tests results becomes
> non-independent. It will depends on the kernel (overlayfs module) user are
> running the test. Imaging if user want to test the latest fsck.overlay
> on the old kernel which contains a compatible feature xattr fsck.overlay
> know but the kernel don't, we will get the unexpected result. Or maybe
> we can add some _require_xxx_feature() helper to enforce user doing test
> on the kernel which supports the specified feature?
>

I think the only sane choice is for this test to relax the expectation of 0
exit code to "correct" exit code (i.e. _overlay_repair_dirs()) for the "Valid"
test cases.

Maybe the only fsck run that we are fine with expecting 0 exit code is
-n run. As you can see this is common practice for e2fsck:
e2fsck -fn "${SCRATCH_DEV}" >> $seqres.full 2>&1 || _fail "fsck should not fail"

Thanks,
Amir.