[PATCH 0/3] commands/test: some improvements

[PATCH 0/3] commands/test: some improvements

[Uwe Kleine-König]

Hello,

my main intention was to implement -b and -c, the other two patches just
fix some stuff I noticed while doing so.

One thing I noticed is that the check for

	strlen(ap[1])

in the handling of -e and others is wrong.

This makes

	test -e ''

return success, but

	test -z something -o -e ''

fails. I don't know for sure how to fix this as the empty string might
mean "." sometimes?

Best regards
Uwe

Uwe Kleine-König (3):
  commands/test: Bail out on incomplete command line options
  commands/test: Improve option parsing to handle "]" less special
  commands/test: Implement -b and -c to test for character and block
    devices

 commands/test.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

-- 
2.24.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] commands/test: Bail out on incomplete command line options

[Uwe Kleine-König]

This makes test emit an error (and fail) on e.g.

	test -f

and also on unimplemented options like

	test -c /dev/null

.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 commands/test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/commands/test.c b/commands/test.c
index c4f493860f14..ab108fc845d5 100644
--- a/commands/test.c
+++ b/commands/test.c
@@ -208,6 +208,11 @@ static int do_test(int argc, char *argv[])
 			}
 		}
 
+		if (left < adv) {
+			printf("test: failed to parse arguments\n");
+			return 1;
+		}
+
 		if (last_cmp == 0)
 			expr = last_expr || expr;
 		else if (last_cmp == 1)
-- 
2.24.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] commands/test: Bail out on incomplete command line options

[Sascha Hauer]

On Tue, Jan 28, 2020 at 02:07:59PM +0100, Uwe Kleine-König wrote:
> This makes test emit an error (and fail) on e.g.
> 
> 	test -f

test -f in bash returns 0 as well and also doesn't emit an error.

> 
> and also on unimplemented options like
> 
> 	test -c /dev/null

test -H /foo/bar in bash says "bash: test: -H: unary operator expected".

I am not sure how relevant this is, but the behaviour you introduce is
not consistent to bash.

Sascha


> 
> .
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  commands/test.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/commands/test.c b/commands/test.c
> index c4f493860f14..ab108fc845d5 100644
> --- a/commands/test.c
> +++ b/commands/test.c
> @@ -208,6 +208,11 @@ static int do_test(int argc, char *argv[])
>  			}
>  		}
>  
> +		if (left < adv) {
> +			printf("test: failed to parse arguments\n");
> +			return 1;
> +		}
> +
>  		if (last_cmp == 0)
>  			expr = last_expr || expr;
>  		else if (last_cmp == 1)
> -- 
> 2.24.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] commands/test: Bail out on incomplete command line options

[Uwe Kleine-König]

On Mon, Feb 03, 2020 at 08:57:07AM +0100, Sascha Hauer wrote:
> On Tue, Jan 28, 2020 at 02:07:59PM +0100, Uwe Kleine-König wrote:
> > This makes test emit an error (and fail) on e.g.
> > 
> > 	test -f
> 
> test -f in bash returns 0 as well and also doesn't emit an error.

Ah, that's because this is implicitly

	test -n -f

That the same happens in barebox is only an accident I would say. (And
note that currently

	test ''

returns success, too, which is different from bash's test behaviour.)

> > and also on unimplemented options like
> > 
> > 	test -c /dev/null
> 
> test -H /foo/bar in bash says "bash: test: -H: unary operator expected".

That is (again) because "-H" "/dev/null" is implicitly "-n" "-H"
"/dev/null" and then there is no sane interpretation of the last
argument.

	test -H -a -f

succeeds because both "-H" and "-f" are non-zero. I'm not sure if trying
to be that clever(?) is a good idea. (Also bash's test isn't without
shortages, for example

	$ test -f -a -a -f
	bash: test: argument expected

while this is an obviously fine test if a file called "-a" exists and
"-f" is not zero-length. Also note that coreutil's test behaves differently
than bash's on for example

	test -f -a -f
	
. (bash returns success, coreutil's test wails about "extra argument
‘-f’".))

> I am not sure how relevant this is, but the behaviour you introduce is
> not consistent to bash.

While I admit I wasn't aware how strange the other test variants are, I
think trying to be as smart as bash's and coreutil's test isn't very
sensible. So I think requesting that the caller explicitly uses "-n" for
testing an argument for being non-empty is a sane idea. Not only because
it catches (probably) errors like:

	somestring=""
	if test -n $somestring; then echo some string is nonempty; fi

but also to keep the parser simpler (and the result less surprising).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] commands/test: Improve option parsing to handle "]" less special

[Uwe Kleine-König]

Testing for *ap[1] != ']' is bogus during option processing. Instead
test if there are options left to be processed.

This fixes the return value for e.g.

	test -z ']lala'

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 commands/test.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/commands/test.c b/commands/test.c
index ab108fc845d5..9070e4907496 100644
--- a/commands/test.c
+++ b/commands/test.c
@@ -129,8 +129,11 @@ static int do_test(int argc, char *argv[])
 		case OPT_ZERO:
 		case OPT_NONZERO:
 			adv = 2;
+			if (left < 2)
+				break;
 			zero = 1;
-			if (ap[1] && *ap[1] != ']' && strlen(ap[1]))
+
+			if (strlen(ap[1]))
 				zero = 0;
 
 			expr = (opt == OPT_ZERO) ? zero : !zero;
@@ -141,7 +144,9 @@ static int do_test(int argc, char *argv[])
 		case OPT_EXISTS:
 		case OPT_SYMBOLIC_LINK:
 			adv = 2;
-			if (ap[1] && *ap[1] != ']' && strlen(ap[1])) {
+			if (left < 2)
+				break;
+			if (strlen(ap[1])) {
 				expr = (opt == OPT_SYMBOLIC_LINK ? lstat : stat)(ap[1], &statbuf);
 				if (expr < 0) {
 					expr = 0;
@@ -170,10 +175,8 @@ static int do_test(int argc, char *argv[])
 		/* three argument options */
 		default:
 			adv = 3;
-			if (left < 3) {
-				expr = 1;
+			if (left < 3)
 				break;
-			}
 
 			a = simple_strtol(ap[0], NULL, 0);
 			b = simple_strtol(ap[2], NULL, 0);
-- 
2.24.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] commands/test: Implement -b and -c to test for character and block devices

[Uwe Kleine-König]

These match the same options on coreutil's test(1).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 commands/test.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/commands/test.c b/commands/test.c
index 9070e4907496..86636de1c283 100644
--- a/commands/test.c
+++ b/commands/test.c
@@ -40,6 +40,8 @@ typedef enum {
 	OPT_DIRECTORY,
 	OPT_FILE,
 	OPT_EXISTS,
+	OPT_BLOCK,
+	OPT_CHAR,
 	OPT_SYMBOLIC_LINK,
 	OPT_MAX,
 } test_opts;
@@ -60,6 +62,8 @@ static char *test_options[] = {
 	[OPT_FILE]			= "-f",
 	[OPT_DIRECTORY]			= "-d",
 	[OPT_EXISTS]			= "-e",
+	[OPT_BLOCK]			= "-b",
+	[OPT_CHAR]			= "-c",
 	[OPT_SYMBOLIC_LINK]		= "-L",
 };
 
@@ -142,6 +146,8 @@ static int do_test(int argc, char *argv[])
 		case OPT_FILE:
 		case OPT_DIRECTORY:
 		case OPT_EXISTS:
+		case OPT_BLOCK:
+		case OPT_CHAR:
 		case OPT_SYMBOLIC_LINK:
 			adv = 2;
 			if (left < 2)
@@ -169,6 +175,14 @@ static int do_test(int argc, char *argv[])
 					expr = 1;
 					break;
 				}
+				if (opt == OPT_BLOCK && S_ISBLK(statbuf.st_mode)) {
+					expr = 1;
+					break;
+				}
+				if (opt == OPT_CHAR && S_ISCHR(statbuf.st_mode)) {
+					expr = 1;
+					break;
+				}
 			}
 			break;
 
-- 
2.24.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/3] commands/test: some improvements

[Sascha Hauer]

On Tue, Jan 28, 2020 at 02:07:58PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> my main intention was to implement -b and -c, the other two patches just
> fix some stuff I noticed while doing so.
> 
> One thing I noticed is that the check for
> 
> 	strlen(ap[1])
> 
> in the handling of -e and others is wrong.
> 
> This makes
> 
> 	test -e ''
> 
> return success, but
> 
> 	test -z something -o -e ''
> 
> fails. I don't know for sure how to fix this as the empty string might
> mean "." sometimes?
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (3):
>   commands/test: Bail out on incomplete command line options
>   commands/test: Improve option parsing to handle "]" less special
>   commands/test: Implement -b and -c to test for character and block
>     devices
> 
>  commands/test.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox