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 <jaeg...@kernel.org>
>>> Signed-off-by: Chao Yu <c...@kernel.org>
>>> ---
>>> 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
>> da...@fromorbit.com
>>
> 



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

Reply via email to