| Message ID | 20210804015248.GA22328@andestech.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cgroup/cgroup_regression_test: Fix umount failure | expand |
> Current check inside tst_umount for whehter a device is mounted is ambiguous. > Fix it by checking for the existance of the exact mount point the fs is mounted upon, > and use the mount point instead of the device to do the umount process. > This patch make tst_umount umount TST_MNTPOINT by default, > and reject any argument that does not start with '/'. > Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com> > --- > testcases/lib/tst_test.sh | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh > index c6aa2c487..1060bda2f 100644 > --- a/testcases/lib/tst_test.sh > +++ b/testcases/lib/tst_test.sh > @@ -277,31 +277,35 @@ tst_mount() > tst_umount() > { > - local device="${1:-$TST_DEVICE}" > + local mntpoint="${1:-$TST_MNTPOINT}" > local i=0 > - [ -z "$device" ] && return > + [ -z "$mntpoint" ] && return > - if ! grep -q "$device" /proc/mounts; then > - tst_res TINFO "The $device is not mounted, skipping umount" > + if [[ "$mntpoint" != /* ]]; then [[ ]] is a bashism, it cannot be used. [1] Instead grep must be used to compare regex: if echo "$mntpoint" |grep -q ^/; then This can be changed during merge. Reviewed-by: Petr Vorel <pvorel@suse.cz> [1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-shell-coding-style > + tst_brk TCONF "The $mntpoint is not an absolute path" > + fi > + > + if ! grep -q "${mntpoint%/}" /proc/mounts; then > + tst_res TINFO "The $mntpoint is not mounted upon, skipping umount" > return > fi > while [ "$i" -lt 50 ]; do > - if umount "$device" > /dev/null; then > + if umount "$mntpoint" > /dev/null; then > return > fi > i=$((i+1)) > - tst_res TINFO "umount($device) failed, try $i ..." > + tst_res TINFO "umount($mntpoint) failed, try $i ..." > tst_res TINFO "Likely gvfsd-trash is probing newly mounted "\ > "fs, kill it to speed up tests." > tst_sleep 100ms > done > - tst_res TWARN "Failed to umount($device) after 50 retries" > + tst_res TWARN "Failed to umount($mntpoint) after 50 retries" > } > tst_mkfs()
Hi Leo,
as I noted at forth patch, some tests use tst_umount with the default parameter.
Thus we should set the default TST_MNTPOINT to use $PWD as well.
I'd also add quotes around $mntpoint in TCONF/TINFO messages in tst_mount() as
the original message was a bit confusing when the mountpoint was called
'mntpoint' (TWARN: The mntpoint is not an absolute path).
Kind regards,
Petr
+++ testcases/lib/tst_test.sh
@@ -279,31 +279,35 @@ tst_mount()
tst_umount()
{
- local device="${1:-$TST_DEVICE}"
+ local mntpoint="${1:-$TST_MNTPOINT}"
local i=0
- [ -z "$device" ] && return
+ [ -z "$mntpoint" ] && return
- if ! grep -q "$device" /proc/mounts; then
- tst_res TINFO "The $device is not mounted, skipping umount"
+ if ! echo "$mntpoint" | grep -q ^/; then
+ tst_brk TCONF "The '$mntpoint' is not an absolute path"
+ fi
+
+ if ! grep -q "${mntpoint%/}" /proc/mounts; then
+ tst_res TINFO "The '$mntpoint' is not mounted upon, skipping umount"
return
fi
while [ "$i" -lt 50 ]; do
- if umount "$device" > /dev/null; then
+ if umount "$mntpoint" > /dev/null; then
return
fi
i=$((i+1))
- tst_res TINFO "umount($device) failed, try $i ..."
+ tst_res TINFO "umount($mntpoint) failed, try $i ..."
tst_res TINFO "Likely gvfsd-trash is probing newly mounted "\
"fs, kill it to speed up tests."
tst_sleep 100ms
done
- tst_res TWARN "Failed to umount($device) after 50 retries"
+ tst_res TWARN "Failed to umount($mntpoint) after 50 retries"
}
tst_mkfs()
@@ -628,7 +632,7 @@ tst_run()
cd "$TST_TMPDIR"
fi
- TST_MNTPOINT="${TST_MNTPOINT:-mntpoint}"
+ TST_MNTPOINT="${TST_MNTPOINT:-$PWD/mntpoint}"
if [ "$TST_NEEDS_DEVICE" = 1 ]; then
TST_DEVICE=$(tst_device acquire)
Hi Petr, Understood! Thanks for explaining everything! I will send a v7 soon including all the modifications needed. Best regards, Leo On Mon, Aug 23, 2021 at 04:19:49PM +0200, Petr Vorel wrote: > Hi Leo, > > as I noted at forth patch, some tests use tst_umount with the default parameter. > Thus we should set the default TST_MNTPOINT to use $PWD as well. > > I'd also add quotes around $mntpoint in TCONF/TINFO messages in tst_mount() as > the original message was a bit confusing when the mountpoint was called > 'mntpoint' (TWARN: The mntpoint is not an absolute path). > > Kind regards, > Petr > > +++ testcases/lib/tst_test.sh > @@ -279,31 +279,35 @@ tst_mount() > > tst_umount() > { > - local device="${1:-$TST_DEVICE}" > + local mntpoint="${1:-$TST_MNTPOINT}" > local i=0 > > - [ -z "$device" ] && return > + [ -z "$mntpoint" ] && return > > - if ! grep -q "$device" /proc/mounts; then > - tst_res TINFO "The $device is not mounted, skipping umount" > + if ! echo "$mntpoint" | grep -q ^/; then > + tst_brk TCONF "The '$mntpoint' is not an absolute path" > + fi > + > + if ! grep -q "${mntpoint%/}" /proc/mounts; then > + tst_res TINFO "The '$mntpoint' is not mounted upon, skipping umount" Will add this in v7! > return > fi > > while [ "$i" -lt 50 ]; do > - if umount "$device" > /dev/null; then > + if umount "$mntpoint" > /dev/null; then > return > fi > > i=$((i+1)) > > - tst_res TINFO "umount($device) failed, try $i ..." > + tst_res TINFO "umount($mntpoint) failed, try $i ..." > tst_res TINFO "Likely gvfsd-trash is probing newly mounted "\ > "fs, kill it to speed up tests." > > tst_sleep 100ms > done > > - tst_res TWARN "Failed to umount($device) after 50 retries" > + tst_res TWARN "Failed to umount($mntpoint) after 50 retries" > } > > tst_mkfs() > @@ -628,7 +632,7 @@ tst_run() > cd "$TST_TMPDIR" > fi > > - TST_MNTPOINT="${TST_MNTPOINT:-mntpoint}" > + TST_MNTPOINT="${TST_MNTPOINT:-$PWD/mntpoint}" This is a good catch. I think I did notice that df01.sh uses tst_umount as a clean up function, but didn't realize that this should be changed! Thanks for the heads up! > if [ "$TST_NEEDS_DEVICE" = 1 ]; then > > TST_DEVICE=$(tst_device acquire)
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh index c6aa2c487..1060bda2f 100644 --- a/testcases/lib/tst_test.sh +++ b/testcases/lib/tst_test.sh @@ -277,31 +277,35 @@ tst_mount() tst_umount() { - local device="${1:-$TST_DEVICE}" + local mntpoint="${1:-$TST_MNTPOINT}" local i=0 - [ -z "$device" ] && return + [ -z "$mntpoint" ] && return - if ! grep -q "$device" /proc/mounts; then - tst_res TINFO "The $device is not mounted, skipping umount" + if [[ "$mntpoint" != /* ]]; then + tst_brk TCONF "The $mntpoint is not an absolute path" + fi + + if ! grep -q "${mntpoint%/}" /proc/mounts; then + tst_res TINFO "The $mntpoint is not mounted upon, skipping umount" return fi while [ "$i" -lt 50 ]; do - if umount "$device" > /dev/null; then + if umount "$mntpoint" > /dev/null; then return fi i=$((i+1)) - tst_res TINFO "umount($device) failed, try $i ..." + tst_res TINFO "umount($mntpoint) failed, try $i ..." tst_res TINFO "Likely gvfsd-trash is probing newly mounted "\ "fs, kill it to speed up tests." tst_sleep 100ms done - tst_res TWARN "Failed to umount($device) after 50 retries" + tst_res TWARN "Failed to umount($mntpoint) after 50 retries" } tst_mkfs()
Current check inside tst_umount for whehter a device is mounted is ambiguous. Fix it by checking for the existance of the exact mount point the fs is mounted upon, and use the mount point instead of the device to do the umount process. This patch make tst_umount umount TST_MNTPOINT by default, and reject any argument that does not start with '/'. Signed-off-by: Leo Yu-Chi Liang <ycliang@andestech.com> --- testcases/lib/tst_test.sh | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)