On 3/26/25 08:47, Zorro Lang wrote:
> On Wed, Mar 26, 2025 at 10:20:30AM +1100, Dave Chinner wrote:
>> On Tue, Mar 25, 2025 at 08:58:24PM +0800, Chao Yu wrote:
>>> This is a regression test to check whether fsck can handle corrupted
>>> nlinks correctly, it uses inject.f2fs to inject nlinks w/ wrong value,
>>> and expects fsck.f2fs can detect such corruption and do the repair.
>>>
>>> Cc: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
>>> Signed-off-by: Chao Yu <chao@xxxxxxxxxx>
>>> ---
>>> v5:
>>> - clean up codes suggested by Dave.
>>> tests/f2fs/009 | 141 +++++++++++++++++++++++++++++++++++++++++++++
>>> tests/f2fs/009.out | 2 +
>>> 2 files changed, 143 insertions(+)
>>> create mode 100755 tests/f2fs/009
>>> create mode 100644 tests/f2fs/009.out
>>>
>>> diff --git a/tests/f2fs/009 b/tests/f2fs/009
>>> new file mode 100755
>>> index 00000000..864fdcfb
>>> --- /dev/null
>>> +++ b/tests/f2fs/009
>>> @@ -0,0 +1,141 @@
>>> +#! /bin/bash
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +# Copyright (c) 2025 Chao Yu. All Rights Reserved.
>>> +#
>>> +# FS QA Test No. f2fs/009
>>> +#
>>> +# This is a regression test to check whether fsck can handle corrupted
>>> +# nlinks correctly, it uses inject.f2fs to inject nlinks w/ wrong value,
>>> +# and expects fsck.f2fs can detect such corruption and do the repair.
>>> +#
>>> +. ./common/preamble
>>> +_begin_fstest auto quick
>>> +
>>> +if [ ! -x "$(type -P socket)" ]; then
>>> + _notrun "Couldn't find socket"
>>> +fi
>>
>> Perhaps something like:
>>
>> _require_command $(type -P socket) socket
>
> Good point! Maybe double quotation marks -- "$(type -P socket)" is
> helpful, due to if socket isn't installed, there will be only one
> argument.
>
>>
>> would be more consistent with all the other code that checks for
Agreed.
>> installed utilities that a test requires?
>>
>>> +_require_scratch
>>> +_require_command "$F2FS_INJECT_PROG" inject.f2fs
>>> +
>>> +_fixed_by_git_commit f2fs-tools 958cd6e \
>>> + "fsck.f2fs: support to repair corrupted i_links"
>>> +
>>> +filename=$SCRATCH_MNT/foo
>>> +hardlink=$SCRATCH_MNT/bar
>>> +
>>> +_cleanup()
>>> +{
>>> + if [ -n "$pid" ]; then
>>> + kill $pid &> /dev/null
>>> + wait
>>> + fi
>>> + cd /
>>> + rm -r -f $tmp.*
>>> +}
>>> +
>>> +_inject_and_check()
>>
>> Single leading "_" is reserved for fstests functions, not for local
>> test functions.
Oh, got it.
>>
>> Just call this one "inject_and_test", because that is what it does,
>> and call this one:
>>
>>> +inject_and_check()
>>> +{
>>> + local nlink=$1
>>> + local create_hardlink=$2
>>> + local ino=$3
>>> +
>>> + if [ -z $ino ]; then
>>> + ino=`stat -c '%i' $filename`
>>> + fi
>>> +
>>> + if [ $create_hardlink == 1 ]; then
>>> + ln $filename $hardlink
>>> + fi
>>> +
>>> + _inject_and_check $nlink $ino
>>> +}
>>
>> something like check_links()
>>
>> Otherwise this is a good improvement.
Thanks Dave for all your review and suggestion!
>
> Hi Chao, if you agree with all these changes, and don't need to change more, I can
> help to merge this patchset with above changes. Or you'd like to send a new version?
Zorro, I'm fine w/ all the changes, I'm appreciate for that if you can
help to update the patch!
Thanks,
>
> Thanks,
> Zorro
>
>>
>> -Dave.
>> --
>> Dave Chinner
>> david@xxxxxxxxxxxxx
>>
>
