* [PATCH v2 0/3] fstests: Fix order of _require_scratch* and _require_dm_target @ 2021-09-08 8:37 Shin'ichiro Kawasaki 2021-09-08 8:37 ` [PATCH v2 1/3] generic/{628,629}: " Shin'ichiro Kawasaki ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Shin'ichiro Kawasaki @ 2021-09-08 8:37 UTC (permalink / raw) To: fstests Cc: Naohiro Aota, Johannes Thumshirn, Damien Le Moal, Shinichiro Kawasaki Test cases with dm_target shall call _require_scratch* before _require_dm_target. However, three test cases generic/628, generic/629 and btrfs/146 do not follow this rule and result in unexpected failures. The first patch in this series fixes the failures in generic/628 and generic/629. The second patch fixes it in btrfs/146. The last patch improves _require_dm_target to prevent the failure in the future test cases to be added. The failure was found with generic/628 and generic/629. I checked all existing test cases and found btrfs/146 also has the issue. Changes from v1: * Changed fix approach according to comment on the list * Added fix for btrfs/146 as the 2nd patch * Separated improvement in _require_dm_target as the last patch Shin'ichiro Kawasaki (3): generic/{628,629}: Fix order of _require_scratch* and _require_dm_target btrfs/146: Add _require_scratch_dev_pool before _require_dm_target common/rc: Check call order of _require_dm_target and _require_scratch* common/rc | 3 +++ tests/btrfs/146 | 1 + tests/generic/628 | 2 +- tests/generic/629 | 2 +- 4 files changed, 6 insertions(+), 2 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] generic/{628,629}: Fix order of _require_scratch* and _require_dm_target 2021-09-08 8:37 [PATCH v2 0/3] fstests: Fix order of _require_scratch* and _require_dm_target Shin'ichiro Kawasaki @ 2021-09-08 8:37 ` Shin'ichiro Kawasaki 2021-09-08 17:04 ` Johannes Thumshirn 2021-09-08 8:37 ` [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target Shin'ichiro Kawasaki 2021-09-08 8:37 ` [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* Shin'ichiro Kawasaki 2 siblings, 1 reply; 11+ messages in thread From: Shin'ichiro Kawasaki @ 2021-09-08 8:37 UTC (permalink / raw) To: fstests Cc: Naohiro Aota, Johannes Thumshirn, Damien Le Moal, Shinichiro Kawasaki Test cases with dm_target shall call _require_scratch* before _require_dm_target to ensure that valid SCRATCH_DEV is available for dm_target. However, the test cases generic/628 and generic/629 call _require_dm_target before _require_scratch*, then unexpected failure was reported when SCRATCH_DEV is not specified. Fix the order of function calls. Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- tests/generic/628 | 2 +- tests/generic/629 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/generic/628 b/tests/generic/628 index 2ccbbbdc..7dc6dfcd 100755 --- a/tests/generic/628 +++ b/tests/generic/628 @@ -29,8 +29,8 @@ _cleanup() # real QA test starts here _supported_fs generic -_require_dm_target error _require_scratch_reflink +_require_dm_target error _require_xfs_io_command "chattr" "s" _require_cp_reflink diff --git a/tests/generic/629 b/tests/generic/629 index abfa90ab..f501555e 100755 --- a/tests/generic/629 +++ b/tests/generic/629 @@ -28,10 +28,10 @@ _cleanup() # real QA test starts here _supported_fs generic +_require_scratch _require_dm_target error _require_xfs_io_command "chattr" "s" _require_xfs_io_command "copy_range" -_require_scratch # Format filesystem and set up quota limits _scratch_mkfs > $seqres.full -- 2.31.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] generic/{628,629}: Fix order of _require_scratch* and _require_dm_target 2021-09-08 8:37 ` [PATCH v2 1/3] generic/{628,629}: " Shin'ichiro Kawasaki @ 2021-09-08 17:04 ` Johannes Thumshirn 0 siblings, 0 replies; 11+ messages in thread From: Johannes Thumshirn @ 2021-09-08 17:04 UTC (permalink / raw) To: Shinichiro Kawasaki, fstests; +Cc: Naohiro Aota, Damien Le Moal Looks good, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target 2021-09-08 8:37 [PATCH v2 0/3] fstests: Fix order of _require_scratch* and _require_dm_target Shin'ichiro Kawasaki 2021-09-08 8:37 ` [PATCH v2 1/3] generic/{628,629}: " Shin'ichiro Kawasaki @ 2021-09-08 8:37 ` Shin'ichiro Kawasaki 2021-09-08 17:05 ` Johannes Thumshirn 2021-09-12 9:30 ` Eryu Guan 2021-09-08 8:37 ` [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* Shin'ichiro Kawasaki 2 siblings, 2 replies; 11+ messages in thread From: Shin'ichiro Kawasaki @ 2021-09-08 8:37 UTC (permalink / raw) To: fstests Cc: Naohiro Aota, Johannes Thumshirn, Damien Le Moal, Shinichiro Kawasaki The test case btrfs/146 calls _require_dm_target which depends on SCRATCH_DEV. The test case assumes that valid devices are set in SCRATCH_DEV_POOL, and one of the devices is propagated to SCRATCH_DEV. However, when SCRATCH_DEV_POOL is not set, valid value is not propagated to SCRATCH_DEV and _require_dm_target causes unexpected test case failure. To avoid the failure, add _require_scratch_dev_pool call before _require_dm_target call to detect invalid SCRATCH_DEV_POOL beforehand. Of note is that the test case replaces SCRATCH_DEV_POOL value internally and calls _require_scratch_dev_pool for the replaced SCRATCH_DEV_POOL. With this fix, the test case will call _require_scratch_dev_pool twice for the original SCRATCH_DEV_POOL and the replaced SCRATCH_DEV_POOL. Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- tests/btrfs/146 | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/btrfs/146 b/tests/btrfs/146 index 64c3513f..eee1a286 100755 --- a/tests/btrfs/146 +++ b/tests/btrfs/146 @@ -26,6 +26,7 @@ _cleanup() # real QA test starts here _supported_fs btrfs +_require_scratch_dev_pool _require_dm_target error _require_test_program fsync-err _require_test_program dmerror -- 2.31.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target 2021-09-08 8:37 ` [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target Shin'ichiro Kawasaki @ 2021-09-08 17:05 ` Johannes Thumshirn 2021-09-12 9:30 ` Eryu Guan 1 sibling, 0 replies; 11+ messages in thread From: Johannes Thumshirn @ 2021-09-08 17:05 UTC (permalink / raw) To: Shinichiro Kawasaki, fstests; +Cc: Naohiro Aota, Damien Le Moal Looks good, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target 2021-09-08 8:37 ` [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target Shin'ichiro Kawasaki 2021-09-08 17:05 ` Johannes Thumshirn @ 2021-09-12 9:30 ` Eryu Guan 1 sibling, 0 replies; 11+ messages in thread From: Eryu Guan @ 2021-09-12 9:30 UTC (permalink / raw) To: Shin'ichiro Kawasaki Cc: fstests, Naohiro Aota, Johannes Thumshirn, Damien Le Moal On Wed, Sep 08, 2021 at 05:37:14PM +0900, Shin'ichiro Kawasaki wrote: > The test case btrfs/146 calls _require_dm_target which depends on > SCRATCH_DEV. The test case assumes that valid devices are set in > SCRATCH_DEV_POOL, and one of the devices is propagated to SCRATCH_DEV. > However, when SCRATCH_DEV_POOL is not set, valid value is not propagated > to SCRATCH_DEV and _require_dm_target causes unexpected test case > failure. To avoid the failure, add _require_scratch_dev_pool call before > _require_dm_target call to detect invalid SCRATCH_DEV_POOL beforehand. > > Of note is that the test case replaces SCRATCH_DEV_POOL value internally > and calls _require_scratch_dev_pool for the replaced SCRATCH_DEV_POOL. > With this fix, the test case will call _require_scratch_dev_pool twice > for the original SCRATCH_DEV_POOL and the replaced SCRATCH_DEV_POOL. > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > tests/btrfs/146 | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tests/btrfs/146 b/tests/btrfs/146 > index 64c3513f..eee1a286 100755 > --- a/tests/btrfs/146 > +++ b/tests/btrfs/146 > @@ -26,6 +26,7 @@ _cleanup() > > # real QA test starts here > _supported_fs btrfs > +_require_scratch_dev_pool There's no need for the test to call _require_scratch_dev_pool twice, it doesn't do any setup work, just to make sure there's scratch pool defined. Move both _require_scratch and _require_scratch_dev_pool up here should be fine. I've fixed it on commit. Thanks, Eryu > _require_dm_target error > _require_test_program fsync-err > _require_test_program dmerror > -- > 2.31.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* 2021-09-08 8:37 [PATCH v2 0/3] fstests: Fix order of _require_scratch* and _require_dm_target Shin'ichiro Kawasaki 2021-09-08 8:37 ` [PATCH v2 1/3] generic/{628,629}: " Shin'ichiro Kawasaki 2021-09-08 8:37 ` [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target Shin'ichiro Kawasaki @ 2021-09-08 8:37 ` Shin'ichiro Kawasaki 2021-09-10 0:48 ` Dave Chinner 2 siblings, 1 reply; 11+ messages in thread From: Shin'ichiro Kawasaki @ 2021-09-08 8:37 UTC (permalink / raw) To: fstests Cc: Naohiro Aota, Johannes Thumshirn, Damien Le Moal, Shinichiro Kawasaki When SCRATCH_DEV is not set and the test case does not call _require_scratch* before _require_dm_target, _require_block_device called from _require_dm_target fails to evaluate SCRATCH_DEV and results in the test case failure. This failure reason is not described in the error message and it takes some time to catch. To catch the failure reason easier, check SCRATCH_DEV in _require_dm_target. If SCRATCH_DEV is not set, fail the test case and print message which requests to fix call order of _require_scratch* and _require_dm_target. This improvement follows what _scratch_shutdown does for _require_scratch_shutdown. Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- common/rc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/rc b/common/rc index dda5da06..cbec8aaa 100644 --- a/common/rc +++ b/common/rc @@ -1971,6 +1971,9 @@ _require_dm_target() # require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF # behaviour + if [ -z "$SCRATCH_DEV" ]; then + _fail "_require_dm_target: call _require_scratch* first in test" + fi _require_block_device $SCRATCH_DEV _require_sane_bdev_flush $SCRATCH_DEV _require_command "$DMSETUP_PROG" dmsetup -- 2.31.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* 2021-09-08 8:37 ` [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* Shin'ichiro Kawasaki @ 2021-09-10 0:48 ` Dave Chinner 2021-09-10 6:34 ` Shinichiro Kawasaki 0 siblings, 1 reply; 11+ messages in thread From: Dave Chinner @ 2021-09-10 0:48 UTC (permalink / raw) To: Shin'ichiro Kawasaki Cc: fstests, Naohiro Aota, Johannes Thumshirn, Damien Le Moal On Wed, Sep 08, 2021 at 05:37:15PM +0900, Shin'ichiro Kawasaki wrote: > When SCRATCH_DEV is not set and the test case does not call > _require_scratch* before _require_dm_target, _require_block_device > called from _require_dm_target fails to evaluate SCRATCH_DEV and > results in the test case failure. This failure reason is not described > in the error message and it takes some time to catch. You should quote the actual failure message here so we have some idea of whether the message that was emitted was appropriate or not without having to go know how the test failed... > To catch the failure reason easier, check SCRATCH_DEV in > _require_dm_target. If SCRATCH_DEV is not set, fail the test case > and print message which requests to fix call order of _require_scratch* > and _require_dm_target. This improvement follows what _scratch_shutdown > does for _require_scratch_shutdown. Also, you don't need to describe the change in the commit message - the patch does that. The first paragraph is all that is needed here as it describes why you want to make the change. > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > common/rc | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/common/rc b/common/rc > index dda5da06..cbec8aaa 100644 > --- a/common/rc > +++ b/common/rc > @@ -1971,6 +1971,9 @@ _require_dm_target() > > # require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF > # behaviour > + if [ -z "$SCRATCH_DEV" ]; then > + _fail "_require_dm_target: call _require_scratch* first in test" > + fi > _require_block_device $SCRATCH_DEV > _require_sane_bdev_flush $SCRATCH_DEV > _require_command "$DMSETUP_PROG" dmsetup That's a notrun case, not a fail. Also, we report the error that has occurred, not how to resolve the problem. That's because we might change behaviour in future and now the error message tells people to do something that is wrong/non-existent. As such, I think the premise this change is based on is not really valid - people running fstests are assumed to have a level of knowledge sufficient to trace a failing test and determine what went wrong from the error reported. i.e. the error message should state what the problem was, not describe a potential solution. Also, this is not the place to check if SCRATCH_DEV is set. The check for a NULL device should be in _require_block_device(). Oh, wait, it already is: _require_block_device() { if [ -z "$1" ]; then echo "Usage: _require_block_device <dev>" 1>&2 exit 1 fi .... } And that's the error message the test emitted that you didn't understand, right? If so, the change here should really be to _require_block_device(). i.e. if [ -z "$1" ]; then _notrun "test requires a block device to be specified" fi A quick scan shows a bunch of similar _requires checks that do similar things with poor error messages and 'exit 1' (e.g. _require_local_device()). _requires rules should call _notrun if the test should not run because of incorrect setup, not 'exit 1'. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* 2021-09-10 0:48 ` Dave Chinner @ 2021-09-10 6:34 ` Shinichiro Kawasaki 2021-09-12 9:17 ` Eryu Guan 0 siblings, 1 reply; 11+ messages in thread From: Shinichiro Kawasaki @ 2021-09-10 6:34 UTC (permalink / raw) To: Dave Chinner; +Cc: fstests, Naohiro Aota, Johannes Thumshirn, Damien Le Moal On Sep 10, 2021 / 10:48, Dave Chinner wrote: > On Wed, Sep 08, 2021 at 05:37:15PM +0900, Shin'ichiro Kawasaki wrote: > > When SCRATCH_DEV is not set and the test case does not call > > _require_scratch* before _require_dm_target, _require_block_device > > called from _require_dm_target fails to evaluate SCRATCH_DEV and > > results in the test case failure. This failure reason is not described > > in the error message and it takes some time to catch. > > You should quote the actual failure message here so we have some > idea of whether the message that was emitted was appropriate or not > without having to go know how the test failed... Sorry about the lack of the infomration. As you found below, the meesage was "Usage: _require_block_device <dev>". > > > To catch the failure reason easier, check SCRATCH_DEV in > > _require_dm_target. If SCRATCH_DEV is not set, fail the test case > > and print message which requests to fix call order of _require_scratch* > > and _require_dm_target. This improvement follows what _scratch_shutdown > > does for _require_scratch_shutdown. > > Also, you don't need to describe the change in the commit message - > the patch does that. The first paragraph is all that is needed here > as it describes why you want to make the change. I see. I will write "why" in the commit message, not "what". (In the past, I was advised to write "what" the patch does, but I think this guide is valid only when the change is complicated). > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > --- > > common/rc | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/common/rc b/common/rc > > index dda5da06..cbec8aaa 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -1971,6 +1971,9 @@ _require_dm_target() > > > > # require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF > > # behaviour > > + if [ -z "$SCRATCH_DEV" ]; then > > + _fail "_require_dm_target: call _require_scratch* first in test" > > + fi > > _require_block_device $SCRATCH_DEV > > _require_sane_bdev_flush $SCRATCH_DEV > > _require_command "$DMSETUP_PROG" dmsetup > > That's a notrun case, not a fail. > > Also, we report the error that has occurred, not how to resolve the > problem. That's because we might change behaviour in future and now > the error message tells people to do something that is > wrong/non-existent. As such, I think the premise this change is based > on is not really valid - people running fstests are assumed to have > a level of knowledge sufficient to trace a failing test and > determine what went wrong from the error reported. i.e. the error > message should state what the problem was, not describe a potential > solution. Thank you for the comment. These are the points I missed. At least I was able to catch the cause, so the improvement I suggested is not a big improvement. > > Also, this is not the place to check if SCRATCH_DEV is set. The > check for a NULL device should be in _require_block_device(). Oh, > wait, it already is: > > _require_block_device() > { > if [ -z "$1" ]; then > echo "Usage: _require_block_device <dev>" 1>&2 > exit 1 > fi > .... > } > > And that's the error message the test emitted that you didn't > understand, right? Right :) > > If so, the change here should really be to _require_block_device(). > i.e. > > if [ -z "$1" ]; then > _notrun "test requires a block device to be specified" > fi > > A quick scan shows a bunch of similar _requires checks that do > similar things with poor error messages and 'exit 1' (e.g. > _require_local_device()). _requires rules should call _notrun if the > test should not run because of incorrect setup, not 'exit 1'. Thank you for your thoughts. I walked through _require_* bash functions in common/, and listed 20 functions below, which call 'exit 1', _fail, or 'return 1' for its argument check failure: --- list start --- common/rc _require_scratch_size _require_scratch_size_nocheck _require_command * _require_block_device * _require_local_device * _require_zoned_device * _require_non_zoned_device * _require_scratch_ext4_feature _require_xfs_io_command _require_fio _require_batched_discard * _require_chattr _require_fs_sysfs _require_scratch_feature common/btrfs _require_btrfs_mkfs_feature _require_btrfs_fs_feature common/xfs _require_xfs_db_command _require_xfs_spaceman_command common/encrypt _require_encryption_policy_support (checks arguments passed from _require_scratch_encryption) common/rnameat2 _require_renameat2 --- list end --- Many of the functions above check arguments not for incorrect setup, but for call in test cases with invalid arguments. 6 functions of them with * in the list check arguments for the incorrect setups, such as DEBUGFS_PROG, SCRATCH_DEV or SCRATCH_MNT. So I suggest to modify these functions to improve error messages and call "_notrun". What do you think about this? -- Best Regards, Shin'ichiro Kawasaki ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* 2021-09-10 6:34 ` Shinichiro Kawasaki @ 2021-09-12 9:17 ` Eryu Guan 2021-09-12 23:28 ` Shinichiro Kawasaki 0 siblings, 1 reply; 11+ messages in thread From: Eryu Guan @ 2021-09-12 9:17 UTC (permalink / raw) To: Shinichiro Kawasaki Cc: Dave Chinner, fstests, Naohiro Aota, Johannes Thumshirn, Damien Le Moal On Fri, Sep 10, 2021 at 06:34:05AM +0000, Shinichiro Kawasaki wrote: > On Sep 10, 2021 / 10:48, Dave Chinner wrote: > > On Wed, Sep 08, 2021 at 05:37:15PM +0900, Shin'ichiro Kawasaki wrote: > > > When SCRATCH_DEV is not set and the test case does not call > > > _require_scratch* before _require_dm_target, _require_block_device > > > called from _require_dm_target fails to evaluate SCRATCH_DEV and > > > results in the test case failure. This failure reason is not described > > > in the error message and it takes some time to catch. > > > > You should quote the actual failure message here so we have some > > idea of whether the message that was emitted was appropriate or not > > without having to go know how the test failed... > > Sorry about the lack of the infomration. As you found below, the meesage was > "Usage: _require_block_device <dev>". > > > > > > To catch the failure reason easier, check SCRATCH_DEV in > > > _require_dm_target. If SCRATCH_DEV is not set, fail the test case > > > and print message which requests to fix call order of _require_scratch* > > > and _require_dm_target. This improvement follows what _scratch_shutdown > > > does for _require_scratch_shutdown. > > > > Also, you don't need to describe the change in the commit message - > > the patch does that. The first paragraph is all that is needed here > > as it describes why you want to make the change. > > I see. I will write "why" in the commit message, not "what". (In the past, I > was advised to write "what" the patch does, but I think this guide is valid > only when the change is complicated). > > > > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > > --- > > > common/rc | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/common/rc b/common/rc > > > index dda5da06..cbec8aaa 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -1971,6 +1971,9 @@ _require_dm_target() > > > > > > # require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF > > > # behaviour > > > + if [ -z "$SCRATCH_DEV" ]; then > > > + _fail "_require_dm_target: call _require_scratch* first in test" > > > + fi > > > _require_block_device $SCRATCH_DEV > > > _require_sane_bdev_flush $SCRATCH_DEV > > > _require_command "$DMSETUP_PROG" dmsetup > > > > That's a notrun case, not a fail. > > > > Also, we report the error that has occurred, not how to resolve the > > problem. That's because we might change behaviour in future and now > > the error message tells people to do something that is > > wrong/non-existent. As such, I think the premise this change is based > > on is not really valid - people running fstests are assumed to have > > a level of knowledge sufficient to trace a failing test and > > determine what went wrong from the error reported. i.e. the error > > message should state what the problem was, not describe a potential > > solution. > > Thank you for the comment. These are the points I missed. At least I was > able to catch the cause, so the improvement I suggested is not a big > improvement. > > > > > Also, this is not the place to check if SCRATCH_DEV is set. The > > check for a NULL device should be in _require_block_device(). Oh, > > wait, it already is: > > > > _require_block_device() > > { > > if [ -z "$1" ]; then > > echo "Usage: _require_block_device <dev>" 1>&2 > > exit 1 > > fi > > .... > > } > > > > And that's the error message the test emitted that you didn't > > understand, right? > > Right :) > > > > > If so, the change here should really be to _require_block_device(). > > i.e. > > > > if [ -z "$1" ]; then > > _notrun "test requires a block device to be specified" > > fi > > > > A quick scan shows a bunch of similar _requires checks that do > > similar things with poor error messages and 'exit 1' (e.g. > > _require_local_device()). _requires rules should call _notrun if the > > test should not run because of incorrect setup, not 'exit 1'. > > Thank you for your thoughts. I walked through _require_* bash functions in > common/, and listed 20 functions below, which call 'exit 1', _fail, or > 'return 1' for its argument check failure: > > --- list start --- > > common/rc > > _require_scratch_size > _require_scratch_size_nocheck > _require_command * > _require_block_device * > _require_local_device * > _require_zoned_device * > _require_non_zoned_device * > _require_scratch_ext4_feature > _require_xfs_io_command > _require_fio > _require_batched_discard * > _require_chattr > _require_fs_sysfs > _require_scratch_feature > > common/btrfs > > _require_btrfs_mkfs_feature > _require_btrfs_fs_feature > > common/xfs > > _require_xfs_db_command > _require_xfs_spaceman_command > > common/encrypt > > _require_encryption_policy_support (checks arguments passed from _require_scratch_encryption) > > common/rnameat2 > > _require_renameat2 > > --- list end --- > > Many of the functions above check arguments not for incorrect setup, but for > call in test cases with invalid arguments. 6 functions of them with * in the > list check arguments for the incorrect setups, such as DEBUGFS_PROG, > SCRATCH_DEV or SCRATCH_MNT. So I suggest to modify these functions to improve > error messages and call "_notrun". What do you think about this? IMO the _fail calls in above _require* rules are indicating function usage errors, which are bugs in the test code. While _notrun indicates a required condition is not met for this test. Thanks, Eryu P.S. I've applied the first two patches, thanks for the fix! > > -- > Best Regards, > Shin'ichiro Kawasaki ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* 2021-09-12 9:17 ` Eryu Guan @ 2021-09-12 23:28 ` Shinichiro Kawasaki 0 siblings, 0 replies; 11+ messages in thread From: Shinichiro Kawasaki @ 2021-09-12 23:28 UTC (permalink / raw) To: Eryu Guan Cc: Dave Chinner, fstests, Naohiro Aota, Johannes Thumshirn, Damien Le Moal On Sep 12, 2021 / 17:17, Eryu Guan wrote: > On Fri, Sep 10, 2021 at 06:34:05AM +0000, Shinichiro Kawasaki wrote: > > On Sep 10, 2021 / 10:48, Dave Chinner wrote: > > > On Wed, Sep 08, 2021 at 05:37:15PM +0900, Shin'ichiro Kawasaki wrote: > > > > When SCRATCH_DEV is not set and the test case does not call > > > > _require_scratch* before _require_dm_target, _require_block_device > > > > called from _require_dm_target fails to evaluate SCRATCH_DEV and > > > > results in the test case failure. This failure reason is not described > > > > in the error message and it takes some time to catch. > > > > > > You should quote the actual failure message here so we have some > > > idea of whether the message that was emitted was appropriate or not > > > without having to go know how the test failed... > > > > Sorry about the lack of the infomration. As you found below, the meesage was > > "Usage: _require_block_device <dev>". > > > > > > > > > To catch the failure reason easier, check SCRATCH_DEV in > > > > _require_dm_target. If SCRATCH_DEV is not set, fail the test case > > > > and print message which requests to fix call order of _require_scratch* > > > > and _require_dm_target. This improvement follows what _scratch_shutdown > > > > does for _require_scratch_shutdown. > > > > > > Also, you don't need to describe the change in the commit message - > > > the patch does that. The first paragraph is all that is needed here > > > as it describes why you want to make the change. > > > > I see. I will write "why" in the commit message, not "what". (In the past, I > > was advised to write "what" the patch does, but I think this guide is valid > > only when the change is complicated). > > > > > > > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > > > --- > > > > common/rc | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/common/rc b/common/rc > > > > index dda5da06..cbec8aaa 100644 > > > > --- a/common/rc > > > > +++ b/common/rc > > > > @@ -1971,6 +1971,9 @@ _require_dm_target() > > > > > > > > # require SCRATCH_DEV to be a valid block device with sane BLKFLSBUF > > > > # behaviour > > > > + if [ -z "$SCRATCH_DEV" ]; then > > > > + _fail "_require_dm_target: call _require_scratch* first in test" > > > > + fi > > > > _require_block_device $SCRATCH_DEV > > > > _require_sane_bdev_flush $SCRATCH_DEV > > > > _require_command "$DMSETUP_PROG" dmsetup > > > > > > That's a notrun case, not a fail. > > > > > > Also, we report the error that has occurred, not how to resolve the > > > problem. That's because we might change behaviour in future and now > > > the error message tells people to do something that is > > > wrong/non-existent. As such, I think the premise this change is based > > > on is not really valid - people running fstests are assumed to have > > > a level of knowledge sufficient to trace a failing test and > > > determine what went wrong from the error reported. i.e. the error > > > message should state what the problem was, not describe a potential > > > solution. > > > > Thank you for the comment. These are the points I missed. At least I was > > able to catch the cause, so the improvement I suggested is not a big > > improvement. > > > > > > > > Also, this is not the place to check if SCRATCH_DEV is set. The > > > check for a NULL device should be in _require_block_device(). Oh, > > > wait, it already is: > > > > > > _require_block_device() > > > { > > > if [ -z "$1" ]; then > > > echo "Usage: _require_block_device <dev>" 1>&2 > > > exit 1 > > > fi > > > .... > > > } > > > > > > And that's the error message the test emitted that you didn't > > > understand, right? > > > > Right :) > > > > > > > > If so, the change here should really be to _require_block_device(). > > > i.e. > > > > > > if [ -z "$1" ]; then > > > _notrun "test requires a block device to be specified" > > > fi > > > > > > A quick scan shows a bunch of similar _requires checks that do > > > similar things with poor error messages and 'exit 1' (e.g. > > > _require_local_device()). _requires rules should call _notrun if the > > > test should not run because of incorrect setup, not 'exit 1'. > > > > Thank you for your thoughts. I walked through _require_* bash functions in > > common/, and listed 20 functions below, which call 'exit 1', _fail, or > > 'return 1' for its argument check failure: > > > > --- list start --- > > > > common/rc > > > > _require_scratch_size > > _require_scratch_size_nocheck > > _require_command * > > _require_block_device * > > _require_local_device * > > _require_zoned_device * > > _require_non_zoned_device * > > _require_scratch_ext4_feature > > _require_xfs_io_command > > _require_fio > > _require_batched_discard * > > _require_chattr > > _require_fs_sysfs > > _require_scratch_feature > > > > common/btrfs > > > > _require_btrfs_mkfs_feature > > _require_btrfs_fs_feature > > > > common/xfs > > > > _require_xfs_db_command > > _require_xfs_spaceman_command > > > > common/encrypt > > > > _require_encryption_policy_support (checks arguments passed from _require_scratch_encryption) > > > > common/rnameat2 > > > > _require_renameat2 > > > > --- list end --- > > > > Many of the functions above check arguments not for incorrect setup, but for > > call in test cases with invalid arguments. 6 functions of them with * in the > > list check arguments for the incorrect setups, such as DEBUGFS_PROG, > > SCRATCH_DEV or SCRATCH_MNT. So I suggest to modify these functions to improve > > error messages and call "_notrun". What do you think about this? > > IMO the _fail calls in above _require* rules are indicating function > usage errors, which are bugs in the test code. While _notrun indicates a > required condition is not met for this test. I see. I think the _require* rules with "exit 1" also indicates the usage errors and the bugs. As Dave pointed out, it is assumed the fstests users have enough skill to identify the bug, then this improvement I suggested don't have much value. I withdraw this suggestion. Dave and Eryu, thank you for the comments. > > Thanks, > Eryu > > P.S. I've applied the first two patches, thanks for the fix! Thanks! -- Best Regards, Shin'ichiro Kawasaki ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-09-12 23:28 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-08 8:37 [PATCH v2 0/3] fstests: Fix order of _require_scratch* and _require_dm_target Shin'ichiro Kawasaki 2021-09-08 8:37 ` [PATCH v2 1/3] generic/{628,629}: " Shin'ichiro Kawasaki 2021-09-08 17:04 ` Johannes Thumshirn 2021-09-08 8:37 ` [PATCH v2 2/3] btrfs/146: Add _require_scratch_dev_pool before _require_dm_target Shin'ichiro Kawasaki 2021-09-08 17:05 ` Johannes Thumshirn 2021-09-12 9:30 ` Eryu Guan 2021-09-08 8:37 ` [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch* Shin'ichiro Kawasaki 2021-09-10 0:48 ` Dave Chinner 2021-09-10 6:34 ` Shinichiro Kawasaki 2021-09-12 9:17 ` Eryu Guan 2021-09-12 23:28 ` Shinichiro Kawasaki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).