* [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs @ 2016-09-26 1:44 Jeff Mahoney 2016-09-26 4:44 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: Jeff Mahoney @ 2016-09-26 1:44 UTC (permalink / raw) To: fstests 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 _scratch_unmount 2> /dev/null # call the overridden mkfs - make sure the FS is built # the same as we'll create it later. diff --git a/common/rc b/common/rc index 13afc6a..4266c18 100644 --- a/common/rc +++ b/common/rc @@ -764,10 +764,18 @@ _scratch_cleanup_files() { case $FSTYP in overlay) + if [ -z "$SCRATCH_DEV" ]; then + echo "\$SCRATCH_DEV is unset." + return 1 + fi # $SCRATCH_DEV is a valid directory in overlay case rm -rf $SCRATCH_DEV/* ;; *) + if [ -z "$SCRATCH_MNT" ]; then + echo "\$SCRATCH_MNT is unset." + return 1 + fi _scratch_mount rm -rf $SCRATCH_MNT/* _scratch_unmount -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs 2016-09-26 1:44 [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs Jeff Mahoney @ 2016-09-26 4:44 ` Dave Chinner 2016-09-26 13:13 ` Jeff Mahoney 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2016-09-26 4:44 UTC (permalink / raw) To: Jeff Mahoney; +Cc: fstests 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 If SCRATCH_DEV/SCRATCH_MNT is not set - which is a valid config - the all that is supposed to happen is that tests which call _require_scratch() should not run. This, in turn should prevent the mkfs->rm problem you mention. The above code in get_next_config() is what needs fixing, not the check code... 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? Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs 2016-09-26 4:44 ` Dave Chinner @ 2016-09-26 13:13 ` Jeff Mahoney 2016-09-26 22:10 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: Jeff Mahoney @ 2016-09-26 13:13 UTC (permalink / raw) To: Dave Chinner; +Cc: fstests [-- Attachment #1.1: Type: text/plain, Size: 2971 bytes --] 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" > 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 #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 881 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs 2016-09-26 13:13 ` Jeff Mahoney @ 2016-09-26 22:10 ` Dave Chinner 2016-09-27 14:48 ` Jeff Mahoney 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2016-09-26 22:10 UTC (permalink / raw) To: Jeff Mahoney; +Cc: fstests 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. > > 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. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs 2016-09-26 22:10 ` Dave Chinner @ 2016-09-27 14:48 ` Jeff Mahoney 0 siblings, 0 replies; 5+ messages in thread From: Jeff Mahoney @ 2016-09-27 14:48 UTC (permalink / raw) To: Dave Chinner; +Cc: fstests [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-27 14:48 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-26 1:44 [PATCH] check: check SCRATCH_MNT before calling _scratch_mkfs Jeff Mahoney 2016-09-26 4:44 ` Dave Chinner 2016-09-26 13:13 ` Jeff Mahoney 2016-09-26 22:10 ` Dave Chinner 2016-09-27 14:48 ` Jeff Mahoney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox