fstests.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Cc: fstests@vger.kernel.org, Naohiro Aota <naohiro.aota@wdc.com>,
	Johannes Thumshirn <johannes.thumshirn@wdc.com>,
	Damien Le Moal <Damien.LeMoal@wdc.com>
Subject: Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch*
Date: Fri, 10 Sep 2021 10:48:57 +1000	[thread overview]
Message-ID: <20210910004857.GI1756565@dread.disaster.area> (raw)
In-Reply-To: <20210908083715.1831067-4-shinichiro.kawasaki@wdc.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

  reply	other threads:[~2021-09-10  0:51 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2021-09-10  6:34     ` Shinichiro Kawasaki
2021-09-12  9:17       ` Eryu Guan
2021-09-12 23:28         ` Shinichiro Kawasaki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210910004857.GI1756565@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=Damien.LeMoal@wdc.com \
    --cc=fstests@vger.kernel.org \
    --cc=johannes.thumshirn@wdc.com \
    --cc=naohiro.aota@wdc.com \
    --cc=shinichiro.kawasaki@wdc.com \
    --subject='Re: [PATCH v2 3/3] common/rc: Check call order of _require_dm_target and _require_scratch*' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).