Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3307 bytes --]

On 9/26/16 6:10 PM, Dave Chinner wrote:
> On Mon, Sep 26, 2016 at 09:13:37AM -0400, Jeff Mahoney wrote:
>> On 9/26/16 12:44 AM, Dave Chinner wrote:
>>> On Sun, Sep 25, 2016 at 09:44:13PM -0400, Jeff Mahoney wrote:
>>>> On NFS or Overlayfs, "mkfs" turns into rm -rf $SCRATCH_MNT/*
>>>>
>>>> There is no warning/error in check if SCRATCH_MNT is unset.
>>>>
>>>> Also add the checks to _scratch_cleanup_files.
>>>>
>>>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>>>> ---
>>>>  check     | 12 ++++++++++++
>>>>  common/rc |  8 ++++++++
>>>>  2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/check b/check
>>>> index 69341d8..b22d2df 100755
>>>> --- a/check
>>>> +++ b/check
>>>> @@ -512,6 +512,18 @@ for section in $HOST_OPTIONS_SECTIONS; do
>>>>  	needwrap=true
>>>>  
>>>>  	if [ ! -z "$SCRATCH_DEV" ]; then
>>>> +	   if [ -z "$SCRATCH_MNT" ]
>>>> +	    then
>>>> +		echo "\$SCRATCH_MNT is unset"
>>>> +		status=1
>>>> +		exit
>>>> +	    fi
>>>> +	   if [ ! -d "$SCRATCH_MNT" ]
>>>> +	    then
>>>> +		echo "\$SCRATCH_MNT is not a dir"
>>>> +		status=1
>>>> +		exit
>>>> +	    fi
>>>
>>> That is supposed to be checked in get_next_config() at the start of
>>> the loop. It runs these checks on the scratch config:
>>>
>>> 	_check_device SCRATCH_DEV optional $SCRATCH_DEV
>>> 	if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then
>>> 		echo "common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not a directory"
>>> 		exit 1
>>> 	fi
>>
>> But that only presents an error if SCRATCH_MNT isn't empty.  If it is,
>> we skip happily past.
>>
>>> If SCRATCH_DEV/SCRATCH_MNT is not set - which is a valid config -
>>
>> SCRATCH_DEV without SCRATCH_MNT isn't a valid config, though.  That
>> ./check assumes that SCRATCH_DEV being valid means SCRATCH_MNT is too is
>> the source of the problem.  The test in get_next_config would prevent
>> the problem if we replaced the ! -z "$SCRATCH_MNT" with ! -z "$SCRATCH_DEV"
> 
> Because SCRATCH_DEV is optional, the check for SCRATCH_MNT in
> get_next_config() needs to take that into account. i.e. if
> SCRATCH_DEV is set, then SCRATCH_MNT must be set, too.

Yep.  I'll post an updated patch that does that.

>>> the all that is supposed to happen is that tests which call
>>> _require_scratch() should not run. This, in turn should prevent
>>
>> ... but _require_scratch doesn't run in ./check.  The individual test
>> cases are safe because _require_scratch runs there and does check.
>> ./check just checks to see if $SCRATCH_DEV is set and then calls
>> _scratch_mkfs without checking $SCRATCH_MNT.  Since _scratch_mkfs
>> doesn't check either, boom.
> 
> Yes, I know. That's why it should be checked in get_next_config() -
> it is supposed to catch any config errors before they get used
> anywhere.
> 
> It makes no sense to sprinkle random "is the config valid" checks
> throughout the code. We should validate the config once - and once
> only - before we run anything. Then we can assume (correctly) the
> config is valid everywhere else.

In general, I agree.  But when the end result for a missing value is rm
-rf /*, I don't think it's overkill.  A mkfs with a missing value isn't
going to accidentally mkfs everything.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]