[PATCH 0/2] fstests: help user to troubleshoot the re-mount issue quickly

[PATCH 0/2] fstests: help user to troubleshoot the re-mount issue quickly

[Jia He]

When I tried to fix a mount issue of tests/xfs/003 failure, I found the 
root cause is my mistake that I added "/" to the end of TEST_DIR, eg.
export TEST_DIR=/root/xfstests/test/
This patch set is to let user find the correct cause quickly

Jia He (2):
  fstests: comments to prevent from adding "/" to the end of 2 environment variables
  fstests: give user friendly prompts for already mounted dir

 common/rc                | 8 ++++++--
 configs/example.config   | 1 +
 configs/localhost.config | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH 1/2] fstests: comments to prevent from adding "/" to the end of 2 environment variables

[Jia He]

This adds comments to prevent user from adding "/" to the end of TEST_DIR and 
SCRATCH_MNT

Signed-off-by: Jia He <hejianet@gmail.com>
---
 configs/example.config   | 1 +
 configs/localhost.config | 1 +
 2 files changed, 2 insertions(+)

diff --git a/configs/example.config b/configs/example.config
index 7b6e142..cf8f08e 100644
--- a/configs/example.config
+++ b/configs/example.config
@@ -1,5 +1,6 @@
 # Example config file with all typical device options set for full
 # XFS kernel, progs and dump testing.
+# please do NOT add "/" to end of TEST_DIR and SCRATCH_MNT
 MODULAR=0
 SCRATCH_MNT=/mnt/scratch
 SCRATCH_DEV=/dev/sdb5
diff --git a/configs/localhost.config b/configs/localhost.config
index f47e694..a7b9fe3 100644
--- a/configs/localhost.config
+++ b/configs/localhost.config
@@ -1,5 +1,6 @@
 # default localhost file, set up for virtual machines using KVM and
 # virtio for it's block devices
+# Please do NOT add "/" to the end of TEST_DIR and SCRATCH_MNT
 #
 MODULAR=0
 TEST_DIR=/mnt/test
-- 
2.1.4

Re: [PATCH 1/2] fstests: comments to prevent from adding "/" to the end of 2 environment variables

[Eryu Guan]

On Thu, Jan 07, 2016 at 02:37:28PM +0800, Jia He wrote:
> This adds comments to prevent user from adding "/" to the end of TEST_DIR and 
> SCRATCH_MNT

Instead of adding comments, how about removing the trailing "/" in the
code, something like:

diff --git a/common/config b/common/config
index e82d279..cb34fd7 100644
--- a/common/config
+++ b/common/config
@@ -551,5 +551,10 @@ if [ -z "$CONFIG_INCLUDED" ]; then
 	[ -z "$FSCK_OPTIONS" ] && _fsck_opts
 fi
 
+# canonicalize the mount points
+# this follows symlinks and removes all trailing "/"s
+export TEST_DIR=`readlink -e "$TEST_DIR"`
+export SCRATCH_MNT=`readlink -e "$SCRATCH_MNT"`
+
 # make sure this script returns success
 /bin/true

Thanks,
Eryu

Re: [PATCH 1/2] fstests: comments to prevent from adding "/" to the end of 2 environment variables

[hejianet]

Hi Eryu Guan
Thanks for the comments,  reasonable to me.
I will add it into v2 patch together with other
在 1/7/16 6:27 PM, Eryu Guan 写道:
> On Thu, Jan 07, 2016 at 02:37:28PM +0800, Jia He wrote:
>> This adds comments to prevent user from adding "/" to the end of TEST_DIR and
>> SCRATCH_MNT
> Instead of adding comments, how about removing the trailing "/" in the
> code, something like:
>
> diff --git a/common/config b/common/config
> index e82d279..cb34fd7 100644
> --- a/common/config
> +++ b/common/config
> @@ -551,5 +551,10 @@ if [ -z "$CONFIG_INCLUDED" ]; then
>   	[ -z "$FSCK_OPTIONS" ] && _fsck_opts
>   fi
>   
> +# canonicalize the mount points
> +# this follows symlinks and removes all trailing "/"s
> +export TEST_DIR=`readlink -e "$TEST_DIR"`
> +export SCRATCH_MNT=`readlink -e "$SCRATCH_MNT"`
> +
Thanks, but maybe it will empty the invalid path and the user doesn't know
why his TEST_DIR/SCRATCH_MNT are assigned to NULL?

eg.
[root@host ~]# ls /root/not_existed
ls: cannot access /root/not_existed: No such file or directory
[root@host ~]# readlink -e /root/not_existed
[root@host ~]#
>   # make sure this script returns success
>   /bin/true
>
> Thanks,
> Eryu
>

Re: [PATCH 1/2] fstests: comments to prevent from adding "/" to the end of 2 environment variables

[Eryu Guan]

On Thu, Jan 07, 2016 at 10:03:53PM +0800, hejianet wrote:
> Hi Eryu Guan
> Thanks for the comments,  reasonable to me.
> I will add it into v2 patch together with other
> 在 1/7/16 6:27 PM, Eryu Guan 写道:
> >On Thu, Jan 07, 2016 at 02:37:28PM +0800, Jia He wrote:
> >>This adds comments to prevent user from adding "/" to the end of TEST_DIR and
> >>SCRATCH_MNT
> >Instead of adding comments, how about removing the trailing "/" in the
> >code, something like:
> >
> >diff --git a/common/config b/common/config
> >index e82d279..cb34fd7 100644
> >--- a/common/config
> >+++ b/common/config
> >@@ -551,5 +551,10 @@ if [ -z "$CONFIG_INCLUDED" ]; then
> >  	[ -z "$FSCK_OPTIONS" ] && _fsck_opts
> >  fi
> >+# canonicalize the mount points
> >+# this follows symlinks and removes all trailing "/"s
> >+export TEST_DIR=`readlink -e "$TEST_DIR"`
> >+export SCRATCH_MNT=`readlink -e "$SCRATCH_MNT"`
> >+
> Thanks, but maybe it will empty the invalid path and the user doesn't know
> why his TEST_DIR/SCRATCH_MNT are assigned to NULL?

These values have been proved to be a directory in get_next_config(), if
they're not, the test errors out there.

Thanks,
Eryu

Re: [PATCH 1/2] fstests: comments to prevent from adding "/" to the end of 2 environment variables

[hejianet]

Hi Eryu
Yes, you are wright

root@justin-u1504:~/xfstests/xfstests# ./check -d xfs/003
common/config: Error: $TEST_DIR (/root/xfstests/test1) is not a directory

B.R.
Justin

在 1/8/16 5:39 PM, Eryu Guan 写道:
> On Thu, Jan 07, 2016 at 10:03:53PM +0800, hejianet wrote:
>> Hi Eryu Guan
>> Thanks for the comments,  reasonable to me.
>> I will add it into v2 patch together with other
>> 在 1/7/16 6:27 PM, Eryu Guan 写道:
>>> On Thu, Jan 07, 2016 at 02:37:28PM +0800, Jia He wrote:
>>>> This adds comments to prevent user from adding "/" to the end of TEST_DIR and
>>>> SCRATCH_MNT
>>> Instead of adding comments, how about removing the trailing "/" in the
>>> code, something like:
>>>
>>> diff --git a/common/config b/common/config
>>> index e82d279..cb34fd7 100644
>>> --- a/common/config
>>> +++ b/common/config
>>> @@ -551,5 +551,10 @@ if [ -z "$CONFIG_INCLUDED" ]; then
>>>   	[ -z "$FSCK_OPTIONS" ] && _fsck_opts
>>>   fi
>>> +# canonicalize the mount points
>>> +# this follows symlinks and removes all trailing "/"s
>>> +export TEST_DIR=`readlink -e "$TEST_DIR"`
>>> +export SCRATCH_MNT=`readlink -e "$SCRATCH_MNT"`
>>> +
>> Thanks, but maybe it will empty the invalid path and the user doesn't know
>> why his TEST_DIR/SCRATCH_MNT are assigned to NULL?
> These values have been proved to be a directory in get_next_config(), if
> they're not, the test errors out there.
>
> Thanks,
> Eryu
>

[PATCH 2/2] fstests: give user friendly prompts for already mounted dir

[Jia He]

This adds user friendly prompts to output the already mounted line from _mount.
xfstests will do the cleanup (ie. umount) and user can not get the mount directory
information.

Signed-off-by: Jia He <hejianet@gmail.com>
---
 common/rc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index d33e3fb..5b40fb4 100644
--- a/common/rc
+++ b/common/rc
@@ -1271,7 +1271,9 @@ _require_scratch_nocheck()
         # if it's mounted, make sure its on $SCRATCH_MNT
         if ! _mount | grep -F $SCRATCH_DEV | grep -q $SCRATCH_MNT
         then
-            echo "\$SCRATCH_DEV is mounted but not on \$SCRATCH_MNT - aborting"
+            echo "\$SCRATCH_DEV=$SCRATCH_DEV is mounted but not on \$SCRATCH_MNT=$SCRATCH_MNT - aborting"
+            echo "Already mounted result:"
+            _mount | grep -F $SCRATCH_DEV
             exit 1
         fi
         # and then unmount it
@@ -1353,7 +1355,9 @@ _require_test()
         # if it's mounted, make sure its on $TEST_DIR
         if ! _mount | grep -F $TEST_DEV | grep -q $TEST_DIR
         then
-            echo "\$TEST_DEV is mounted but not on \$TEST_DIR - aborting"
+            echo "\$TEST_DEV=$TEST_DEV is mounted but not on \$TEST_DIR=$TEST_DIR - aborting"
+            echo "Already mounted result:"
+            _mount | grep -F $TEST_DEV
             exit 1
         fi
     else
-- 
2.1.4

Re: [PATCH 2/2] fstests: give user friendly prompts for already mounted dir

[Dave Chinner]

On Thu, Jan 07, 2016 at 02:37:29PM +0800, Jia He wrote:
> This adds user friendly prompts to output the already mounted line from _mount.
> xfstests will do the cleanup (ie. umount) and user can not get the mount directory
> information.
> 
> Signed-off-by: Jia He <hejianet@gmail.com>
> ---
>  common/rc | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index d33e3fb..5b40fb4 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1271,7 +1271,9 @@ _require_scratch_nocheck()
>          # if it's mounted, make sure its on $SCRATCH_MNT
>          if ! _mount | grep -F $SCRATCH_DEV | grep -q $SCRATCH_MNT
>          then
> -            echo "\$SCRATCH_DEV is mounted but not on \$SCRATCH_MNT - aborting"
> +            echo "\$SCRATCH_DEV=$SCRATCH_DEV is mounted but not on \$SCRATCH_MNT=$SCRATCH_MNT - aborting"
> +            echo "Already mounted result:"
> +            _mount | grep -F $SCRATCH_DEV
>              exit 1
>          fi

Should use a temporary local variable rather than looking at the
mount table twice. e.g.

	mount_rec=`_mount | grep -F $SCRATCH_DEV`
	echo $mount_rec | grep -q $SCRATCH_MNT
	if [ $? -ne 0 ]; then
		# new error message
		echo $mount_rec
	fi

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] fstests: give user friendly prompts for already mounted dir

[hejianet]

在 1/11/16 7:29 AM, Dave Chinner 写道:
> On Thu, Jan 07, 2016 at 02:37:29PM +0800, Jia He wrote:
>> This adds user friendly prompts to output the already mounted line from _mount.
>> xfstests will do the cleanup (ie. umount) and user can not get the mount directory
>> information.
>>
>> Signed-off-by: Jia He <hejianet@gmail.com>
>> ---
>>   common/rc | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index d33e3fb..5b40fb4 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -1271,7 +1271,9 @@ _require_scratch_nocheck()
>>           # if it's mounted, make sure its on $SCRATCH_MNT
>>           if ! _mount | grep -F $SCRATCH_DEV | grep -q $SCRATCH_MNT
>>           then
>> -            echo "\$SCRATCH_DEV is mounted but not on \$SCRATCH_MNT - aborting"
>> +            echo "\$SCRATCH_DEV=$SCRATCH_DEV is mounted but not on \$SCRATCH_MNT=$SCRATCH_MNT - aborting"
>> +            echo "Already mounted result:"
>> +            _mount | grep -F $SCRATCH_DEV
>>               exit 1
>>           fi
> Should use a temporary local variable rather than looking at the
> mount table twice. e.g.
>
> 	mount_rec=`_mount | grep -F $SCRATCH_DEV`
> 	echo $mount_rec | grep -q $SCRATCH_MNT
> 	if [ $? -ne 0 ]; then
> 		# new error message
> 		echo $mount_rec
> 	fi
Thanks， Dave
v3 is sending.

B.R.
Justin
>
> Cheers,
>
> Dave.