[LTP] [PATCH v1] rwtest: Confirm df is a symlink to busybox

[LTP] [PATCH v1] rwtest: Confirm df is a symlink to busybox

[Mengchi Cheng via ltp]

/bin/df can be a symlink to coreutils. It returns correct info with dir
arguments.
Just checking if df is a symlink will include such cases. Need to make
sure it is linking to busybox before ignoring options.

Signed-off-by: Mengchi Cheng <mengcc@amazon.com>
---
 testcases/kernel/fs/doio/rwtest | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/fs/doio/rwtest b/testcases/kernel/fs/doio/rwtest
index 6725e1426..26659e9d3 100644
--- a/testcases/kernel/fs/doio/rwtest
+++ b/testcases/kernel/fs/doio/rwtest
@@ -329,10 +329,10 @@ do
 		else
 			# If df is a symlink (to busybox) then do not pass the $dir and $dfOpts
 			# parameters because they don't work as expected
-                        if test -h $(which df)
-                           then
-                               dir=""; dfOpts="";
-                        fi
+			if [[ "$(readlink -f "$(which df)")" == *"busybox"* ]]
+			then
+				dir=""; dfOpts="";
+			fi
 
 			blks=$(df $dfOpts $dir |
 			(while read fs blks used avail cap mountpoint
-- 
2.25.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] rwtest: Confirm df is a symlink to busybox

[Petr Vorel]

Hi Mengchi,

> /bin/df can be a symlink to coreutils. It returns correct info with dir
> arguments.
> Just checking if df is a symlink will include such cases. Need to make
> sure it is linking to busybox before ignoring options.

> Signed-off-by: Mengchi Cheng <mengcc@amazon.com>
> ---
>  testcases/kernel/fs/doio/rwtest | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

> diff --git a/testcases/kernel/fs/doio/rwtest b/testcases/kernel/fs/doio/rwtest
> index 6725e1426..26659e9d3 100644
> --- a/testcases/kernel/fs/doio/rwtest
> +++ b/testcases/kernel/fs/doio/rwtest
> @@ -329,10 +329,10 @@ do
>  		else
>  			# If df is a symlink (to busybox) then do not pass the $dir and $dfOpts
>  			# parameters because they don't work as expected
> -                        if test -h $(which df)
> -                           then
> -                               dir=""; dfOpts="";
> -                        fi
> +			if [[ "$(readlink -f "$(which df)")" == *"busybox"* ]]
Could you please test if this works?

			if [ "$(readlink -f "$(which df)")" = "busybox" ]

Although this script uses bash, we generally don't want to use it. Thus it'd be
better to not introduce any bash specific code.

BTW this is very old and very ugly script, we should cleanup it or delete:
https://github.com/linux-test-project/ltp/issues/1110

Kind regards,
Petr

> +			then
> +				dir=""; dfOpts="";
> +			fi

>  			blks=$(df $dfOpts $dir |
>  			(while read fs blks used avail cap mountpoint

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] rwtest: Confirm df is a symlink to busybox

[Petr Vorel]

Hi Mengchi,

Fixes: #1104

(to close https://github.com/linux-test-project/ltp/issues/1104)

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] rwtest: Confirm df is a symlink to busybox

[Mengchi Cheng via ltp]

On Thu, 2023-11-30 10:41:27 +0000, Petr Vorel wrote:
>
> Hi Mengchi,
> 
> > /bin/df can be a symlink to coreutils. It returns correct info with dir
> > arguments.
> > Just checking if df is a symlink will include such cases. Need to make
> > sure it is linking to busybox before ignoring options.
> 
> > Signed-off-by: Mengchi Cheng <mengcc@amazon.com>
> > ---
> >  testcases/kernel/fs/doio/rwtest | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> > diff --git a/testcases/kernel/fs/doio/rwtest b/testcases/kernel/fs/doio/rwtest
> > index 6725e1426..26659e9d3 100644
> > --- a/testcases/kernel/fs/doio/rwtest
> > +++ b/testcases/kernel/fs/doio/rwtest
> > @@ -329,10 +329,10 @@ do
> >  		else
> >  			# If df is a symlink (to busybox) then do not pass the $dir and $dfOpts
> >  			# parameters because they don't work as expected
> > -                        if test -h $(which df)
> > -                           then
> > -                               dir=""; dfOpts="";
> > -                        fi
> > +			if [[ "$(readlink -f "$(which df)")" == *"busybox"* ]]
> Could you please test if this works?
> 
> 			if [ "$(readlink -f "$(which df)")" = "busybox" ]

I just replaced df with a symlink cmd and the string such as zstdmt/zstd in ubuntu.
It does not work..
But below should work
			if echo "$(readlink -f "$(which df)")" | grep -q "busybox"

However, I linked df to /bin/busybox on my device, df -P ${dir} seems fine.
The original code may be for a very old version of busybox.
:/# ls -l /bin/df
lrwxrwxrwx 1 root root 12 2023-11-16 17:49 /bin/df -> /bin/busybox
:/# df -P tmp
Filesystem           1024-blocks    Used Available Capacity Mounted on
tmpfs                   280848         4    280844   0% /tmp
:/# /usr/bin/df.coreutils -P tmp
Filesystem     1024-blocks  Used Available Capacity Mounted on
tmpfs               280848     4    280844       1% /tmp

My busybox version is v1.35.0. It might be ok to remove the check completely.


Best,
Mengchi
 
> 
> Although this script uses bash, we generally don't want to use it. Thus it'd be
> better to not introduce any bash specific code.
> 
> BTW this is very old and very ugly script, we should cleanup it or delete:
> https://github.com/linux-test-project/ltp/issues/1110
> 
> Kind regards,
> Petr
> 
> > +			then
> > +				dir=""; dfOpts="";
> > +			fi
> 
> >  			blks=$(df $dfOpts $dir |
> >  			(while read fs blks used avail cap mountpoint
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp
> 

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1] rwtest: Confirm df is a symlink to busybox

[Petr Vorel]

> On Thu, 2023-11-30 10:41:27 +0000, Petr Vorel wrote:

> > Hi Mengchi,

> > > /bin/df can be a symlink to coreutils. It returns correct info with dir
> > > arguments.
> > > Just checking if df is a symlink will include such cases. Need to make
> > > sure it is linking to busybox before ignoring options.

> > > Signed-off-by: Mengchi Cheng <mengcc@amazon.com>
> > > ---
> > >  testcases/kernel/fs/doio/rwtest | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)

> > > diff --git a/testcases/kernel/fs/doio/rwtest b/testcases/kernel/fs/doio/rwtest
> > > index 6725e1426..26659e9d3 100644
> > > --- a/testcases/kernel/fs/doio/rwtest
> > > +++ b/testcases/kernel/fs/doio/rwtest
> > > @@ -329,10 +329,10 @@ do
> > >  		else
> > >  			# If df is a symlink (to busybox) then do not pass the $dir and $dfOpts
> > >  			# parameters because they don't work as expected
> > > -                        if test -h $(which df)
> > > -                           then
> > > -                               dir=""; dfOpts="";
> > > -                        fi
> > > +			if [[ "$(readlink -f "$(which df)")" == *"busybox"* ]]
> > Could you please test if this works?

> > 			if [ "$(readlink -f "$(which df)")" = "busybox" ]

> I just replaced df with a symlink cmd and the string such as zstdmt/zstd in ubuntu.
> It does not work..
> But below should work
> 			if echo "$(readlink -f "$(which df)")" | grep -q "busybox"

Yes, this would work, but this would be better:

if df --version 2>&1 | grep -q 'BusyBox'

this is way simpler and this detection is already used elsewhere.

BTW how can happen that df is symlink on coreutils?
They provide a binary, right?

> However, I linked df to /bin/busybox on my device, df -P ${dir} seems fine.
> The original code may be for a very old version of busybox.
> :/# ls -l /bin/df
> lrwxrwxrwx 1 root root 12 2023-11-16 17:49 /bin/df -> /bin/busybox
> :/# df -P tmp
> Filesystem           1024-blocks    Used Available Capacity Mounted on
> tmpfs                   280848         4    280844   0% /tmp
> :/# /usr/bin/df.coreutils -P tmp
> Filesystem     1024-blocks  Used Available Capacity Mounted on
> tmpfs               280848     4    280844       1% /tmp

> My busybox version is v1.35.0. It might be ok to remove the check completely.

-P was implemented in 2008
https://github.com/mirror/busybox/commit/d66aa3c701ffb83343239e71a8b294407ff5df86

BusyBox has often some features configurable (feature can be disabled), but if I
look correctly -P is not guarded to any config option

https://github.com/mirror/busybox/blob/master/coreutils/df.c

Therefore I'm for removing this option entirely. Please send v2.

Kind regards,
Petr

> Best,
> Mengchi

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp