Re: [PATCH 1/2] common/filter.btrfs: add a new _filter_snapshot

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/4/5 19:55, Anand Jain 写道:
>
>
> On 4/5/24 16:52, Qu Wenruo wrote:
>>
>>
>> 在 2024/4/5 19:15, Anand Jain 写道:
>>> As the newer btrfs-progs have changed the output of the command
>>> "btrfs subvolume snapshot," which is part of the golden output,
>>> create a helper filter to ensure the test cases pass on older
>>> btrfs-progs.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> Can we stop the golden output filter game?
>>
>>  From day one I'm not a big fan of the golden output idea.
>> For snapshot/subvolume creation, we don't really care about what the
>> output is, we only care if there is any error (which would come from
>> stderr).
>>
>> In that case, why not just redirect the stdout to null?
>>
>> To me, if we really care something from the stdout, we can still save it
>> and let bash/awk/grep to process it, like what we did for various test
>> cases, and then save the result to seqres.full.
>>
>
> This is a bug-fix patch; it's not a good idea to change the concept of
> fstests' golden output. Perhaps an RFC patch about your idea can help
> to discuss and achieve consensus.

Even as bug-fix, a simple redirect to seqres.full and remove the
corresponding line from golden output is very valid to me.

In fact, introducing a filter looks very over-engineered in this
particular case.

>
> Thanks, Anand
>
>
>> Thanks,
>> Qu
>>> ---
>>>   common/filter.btrfs | 9 +++++++++
>>>   tests/btrfs/001     | 3 ++-
>>>   tests/btrfs/152     | 6 +++---
>>>   tests/btrfs/168     | 6 +++---
>>>   tests/btrfs/202     | 4 ++--
>>>   tests/btrfs/302     | 4 ++--
>>>   6 files changed, 21 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/common/filter.btrfs b/common/filter.btrfs
>>> index 9ef9676175c9..415ed6dfd088 100644
>>> --- a/common/filter.btrfs
>>> +++ b/common/filter.btrfs
>>> @@ -156,5 +156,14 @@ _filter_device_add()
>>>
>>>   }
>>>
>>> +_filter_snapshot()
>>> +{
>>> +    # btrfs-progs commit 5f87b467a9e7 ("btrfs-progs: subvolume:
>>> output the
>>> +    # prompt line only when the ioctl succeeded") changed the output
>>> for
>>> +    # btrfs subvolume snapshot, ensure that the latest fstests
>>> continue to
>>> +    # work on older btrfs-progs without the above commit.
>>> +    _filter_scratch | sed -e "s/Create a/Create/g"
>>> +}
>>> +
>>>   # make sure this script returns success
>>>   /bin/true
>>> diff --git a/tests/btrfs/001 b/tests/btrfs/001
>>> index 6c2639990373..cfcf2ade4590 100755
>>> --- a/tests/btrfs/001
>>> +++ b/tests/btrfs/001
>>> @@ -26,7 +26,8 @@ dd if=/dev/zero of=$SCRATCH_MNT/foo bs=1M count=1
>>> &> /dev/null
>>>   echo "List root dir"
>>>   ls $SCRATCH_MNT
>>>   echo "Creating snapshot of root dir"
>>> -$BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap |
>>> _filter_scratch
>>> +$BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap | \
>>> +                            _filter_snapshot
>>>   echo "List root dir after snapshot"
>>>   ls $SCRATCH_MNT
>>>   echo "List snapshot dir"
>>> diff --git a/tests/btrfs/152 b/tests/btrfs/152
>>> index 75f576c3cfca..b89fe361e84e 100755
>>> --- a/tests/btrfs/152
>>> +++ b/tests/btrfs/152
>>> @@ -11,7 +11,7 @@
>>>   _begin_fstest auto quick metadata qgroup send
>>>
>>>   # Import common functions.
>>> -. ./common/filter
>>> +. ./common/filter.btrfs
>>>
>>>   # real QA test starts here
>>>   _supported_fs btrfs
>>> @@ -32,9 +32,9 @@ touch $SCRATCH_MNT/subvol{1,2}/foo
>>>
>>>   # Create base snapshots and send them
>>>   $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/subvol1 \
>>> -    $SCRATCH_MNT/subvol1/.snapshots/1 | _filter_scratch
>>> +    $SCRATCH_MNT/subvol1/.snapshots/1 | _filter_snapshot
>>>   $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/subvol2 \
>>> -    $SCRATCH_MNT/subvol2/.snapshots/1 | _filter_scratch
>>> +    $SCRATCH_MNT/subvol2/.snapshots/1 | _filter_snapshot
>>>   for recv in recv1_1 recv1_2 recv2_1 recv2_2; do
>>>       $BTRFS_UTIL_PROG send $SCRATCH_MNT/subvol1/.snapshots/1 2>
>>> /dev/null | \
>>>           $BTRFS_UTIL_PROG receive $SCRATCH_MNT/${recv} |
>>> _filter_scratch
>>> diff --git a/tests/btrfs/168 b/tests/btrfs/168
>>> index acc58b51ee39..78bc9b8f81bb 100755
>>> --- a/tests/btrfs/168
>>> +++ b/tests/btrfs/168
>>> @@ -20,7 +20,7 @@ _cleanup()
>>>   }
>>>
>>>   # Import common functions.
>>> -. ./common/filter
>>> +. ./common/filter.btrfs
>>>
>>>   # real QA test starts here
>>>   _supported_fs btrfs
>>> @@ -74,7 +74,7 @@ $BTRFS_UTIL_PROG property set $SCRATCH_MNT/sv1 ro
>>> false
>>>   # Create a snapshot of the subvolume, to be used later as the
>>> parent snapshot
>>>   # for an incremental send operation.
>>>   $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/sv1
>>> $SCRATCH_MNT/snap1 \
>>> -    | _filter_scratch
>>> +                            | _filter_snapshot
>>>
>>>   # First do a full send of this snapshot.
>>>   $FSSUM_PROG -A -f -w $send_files_dir/snap1.fssum $SCRATCH_MNT/snap1
>>> @@ -88,7 +88,7 @@ $XFS_IO_PROG -c "pwrite -S 0x19 4K 8K"
>>> $SCRATCH_MNT/sv1/baz >>$seqres.full
>>>   # Create a second snapshot of the subvolume, to be used later as
>>> the send
>>>   # snapshot of an incremental send operation.
>>>   $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/sv1
>>> $SCRATCH_MNT/snap2 \
>>> -    | _filter_scratch
>>> +                            | _filter_snapshot
>>>
>>>   # Temporarily turn the second snapshot to read-write mode and then
>>> open a file
>>>   # descriptor on its foo file.
>>> diff --git a/tests/btrfs/202 b/tests/btrfs/202
>>> index 5f0429f18bf9..57ecbe47c0bb 100755
>>> --- a/tests/btrfs/202
>>> +++ b/tests/btrfs/202
>>> @@ -8,7 +8,7 @@
>>>   . ./common/preamble
>>>   _begin_fstest auto quick subvol snapshot
>>>
>>> -. ./common/filter
>>> +. ./common/filter.btrfs
>>>
>>>   _supported_fs btrfs
>>>   _require_scratch
>>> @@ -28,7 +28,7 @@ _scratch_mount
>>>   $BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/a | _filter_scratch
>>>   $BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/a/b | _filter_scratch
>>>   $BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT/a $SCRATCH_MNT/c \
>>> -    | _filter_scratch
>>> +                            | _filter_snapshot
>>>
>>>   # Need the dummy entry created so that we get the invalid removal
>>> when we rmdir
>>>   ls $SCRATCH_MNT/c/b
>>> diff --git a/tests/btrfs/302 b/tests/btrfs/302
>>> index f3e6044b5251..52d712ac50de 100755
>>> --- a/tests/btrfs/302
>>> +++ b/tests/btrfs/302
>>> @@ -15,7 +15,7 @@
>>>   . ./common/preamble
>>>   _begin_fstest auto quick snapshot subvol
>>>
>>> -. ./common/filter
>>> +. ./common/filter.btrfs
>>>
>>>   _supported_fs btrfs
>>>   _require_scratch
>>> @@ -46,7 +46,7 @@ $FSSUM_PROG -A -f -w $fssum_file $SCRATCH_MNT/subvol
>>>   # Now create a snapshot of the subvolume and make it accessible
>>> from within the
>>>   # subvolume.
>>>   $BTRFS_UTIL_PROG subvolume snapshot -r $SCRATCH_MNT/subvol \
>>> -         $SCRATCH_MNT/subvol/snap | _filter_scratch
>>> +                 $SCRATCH_MNT/subvol/snap | _filter_snapshot
>>>
>>>   # Now unmount and mount again the fs. We want to verify we are able
>>> to read all
>>>   # metadata for the snapshot from disk (no IO failures, etc).
>