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@xxxxxxxx> >> --- >> 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" > 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. > the mkfs->rm problem you mention. The above code in > get_next_config() is what needs fixing, not the check code... In ./check, sure. Fixing get_next_config does work as well. I think I'd like to keep the checks in _scratch_cleanup_files though so we don't accidentally trip over that elsewhere. > How did you actually trip over this? I'm guessing you have a config > problem where you are defining SCRATCH_DEV but not SCRATCH_MNT? > Or you didn't set SCRATCH_DEV_NOT_USED? Yeah, it's definitely a config problem. SCRATCH_DEV was defined but I'd defined SCRATCH_DIR instead of SCRATCH_MNT. Unfortunately, it's one I discovered after "rm -rf /*" made it through to my autofs mount and started working there. -Jeff -- Jeff Mahoney SUSE Labs
Attachment:
signature.asc
Description: OpenPGP digital signature
