Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch*

[Dave Chinner <david@fromorbit.com>]

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