diff mbox

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

Message ID 20160107102723.GD21019@eguan.usersys.redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eryu Guan Jan. 7, 2016, 10:27 a.m. UTC
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:


Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jia He Jan. 7, 2016, 2:03 p.m. UTC | #1
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
>

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eryu Guan Jan. 8, 2016, 9:39 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jia He Jan. 8, 2016, 1:28 p.m. UTC | #3
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
>

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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