[PATCH libgpiod 0/4] tools: tests: fix a few issues in bash scripts

[PATCH libgpiod 0/4] tools: tests: fix a few issues in bash scripts

[Bartosz Golaszewski]

Fix a few issues with tools tests reported by Andy.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Bartosz Golaszewski (4):
      tools: tests: use tabs for indentation consistently
      tools: tests: use $@ instead of $*
      tools: tests: remove unneeded ';' in while loops
      tools: tests: remove dependency on grep

 tools/gpio-tools-test.bash | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)
---
base-commit: 27fe10150f6d5fa78d1a1ef1e922dc8395d1154d
change-id: 20240524-fix-bash-tests-545bf26447b3

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH libgpiod 1/4] tools: tests: use tabs for indentation consistently

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Replace all spaces used for indentation with tabs.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpio-tools-test.bash | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
index b55c5eb..abb2f5d 100755
--- a/tools/gpio-tools-test.bash
+++ b/tools/gpio-tools-test.bash
@@ -203,9 +203,9 @@ dut_run_redirect() {
 
 dut_read_redirect() {
 	output=$(<$SHUNIT_TMPDIR/$DUT_OUTPUT)
-        local ORIG_IFS="$IFS"
-        IFS=$'\n' lines=($output)
-        IFS="$ORIG_IFS"
+	local ORIG_IFS="$IFS"
+	IFS=$'\n' lines=($output)
+	IFS="$ORIG_IFS"
 }
 
 dut_read() {
@@ -269,12 +269,12 @@ dut_wait() {
 }
 
 dut_cleanup() {
-        if [ -n "$DUT_PID" ]
-        then
+	if [ -n "$DUT_PID" ]
+	then
 		kill -SIGTERM $DUT_PID 2> /dev/null
 		wait $DUT_PID || false
-        fi
-        rm -f $SHUNIT_TMPDIR/$DUT_OUTPUT
+	fi
+	rm -f $SHUNIT_TMPDIR/$DUT_OUTPUT
 }
 
 tearDown() {

-- 
2.43.0

[PATCH libgpiod 2/4] tools: tests: use $@ instead of $*

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

$@ does not break up quoted arguments which is what we want in all cases
in the bash test-suite. Use it instead of $*.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpio-tools-test.bash | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
index abb2f5d..8b4f054 100755
--- a/tools/gpio-tools-test.bash
+++ b/tools/gpio-tools-test.bash
@@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
 MIN_KERNEL_VERSION="5.17.4"
 MIN_SHUNIT_VERSION="2.1.8"
 
-# Run the command in $* and fail the test if the command succeeds.
+# Run the command in $@ and fail the test if the command succeeds.
 assert_fail() {
-	$* || return 0
-	fail " '$*': command did not fail as expected"
+	$@ || return 0
+	fail " '$@': command did not fail as expected"
 }
 
 # Check if the string in $2 matches against the pattern in $1.
@@ -71,7 +71,7 @@ gpiosim_chip() {
 
 	mkdir -p $BANKPATH
 
-	for ARG in $*
+	for ARG in $@
 	do
 		local KEY=$(echo $ARG | cut -d"=" -f1)
 		local VAL=$(echo $ARG | cut -d"=" -f2)
@@ -253,7 +253,7 @@ dut_regex_match() {
 }
 
 dut_write() {
-	echo $* >&${COPROC[1]}
+	echo $@ >&${COPROC[1]}
 }
 
 dut_kill() {
@@ -283,7 +283,7 @@ tearDown() {
 }
 
 request_release_line() {
-	$SOURCE_DIR/gpioget -c $* >/dev/null
+	$SOURCE_DIR/gpioget -c $@ >/dev/null
 }
 
 #

-- 
2.43.0

Re: [PATCH libgpiod 2/4] tools: tests: use $@ instead of $*

[Kent Gibson]

On Fri, May 24, 2024 at 08:03:28PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> $@ does not break up quoted arguments which is what we want in all cases
> in the bash test-suite. Use it instead of $*.
>

I believe it needs to be "$@".  Everywhere.

Where do we use quoted arguments/whitespaced parameters?
So this is purely about "good" shell?  In that case why stop here - e.g.
shellcheck picks up a load more "Double quote to prevent splitting/globbing"
and the like.

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  tools/gpio-tools-test.bash | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
> index abb2f5d..8b4f054 100755
> --- a/tools/gpio-tools-test.bash
> +++ b/tools/gpio-tools-test.bash
> @@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
>  MIN_KERNEL_VERSION="5.17.4"
>  MIN_SHUNIT_VERSION="2.1.8"
>
> -# Run the command in $* and fail the test if the command succeeds.
> +# Run the command in $@ and fail the test if the command succeeds.
>  assert_fail() {
> -	$* || return 0
> -	fail " '$*': command did not fail as expected"
> +	$@ || return 0
> +	fail " '$@': command did not fail as expected"
>  }
>
>  # Check if the string in $2 matches against the pattern in $1.
> @@ -71,7 +71,7 @@ gpiosim_chip() {
>
>  	mkdir -p $BANKPATH
>
> -	for ARG in $*
> +	for ARG in $@
>  	do
>  		local KEY=$(echo $ARG | cut -d"=" -f1)
>  		local VAL=$(echo $ARG | cut -d"=" -f2)
> @@ -253,7 +253,7 @@ dut_regex_match() {
>  }
>
>  dut_write() {
> -	echo $* >&${COPROC[1]}
> +	echo $@ >&${COPROC[1]}
>  }
>

Does echo care?  I realise a linter, and that includes Andy, will complain
here, but is there a practical difference?

Cheers,
Kent.

>  dut_kill() {
> @@ -283,7 +283,7 @@ tearDown() {
>  }
>
>  request_release_line() {
> -	$SOURCE_DIR/gpioget -c $* >/dev/null
> +	$SOURCE_DIR/gpioget -c $@ >/dev/null
>  }
>
>  #
>
> --
> 2.43.0
>

Re: [PATCH libgpiod 2/4] tools: tests: use $@ instead of $*

[Bartosz Golaszewski]

On Sat, May 25, 2024 at 3:54 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, May 24, 2024 at 08:03:28PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > $@ does not break up quoted arguments which is what we want in all cases
> > in the bash test-suite. Use it instead of $*.
> >
>
> I believe it needs to be "$@".  Everywhere.
>
> Where do we use quoted arguments/whitespaced parameters?
> So this is purely about "good" shell?  In that case why stop here - e.g.
> shellcheck picks up a load more "Double quote to prevent splitting/globbing"
> and the like.
>

You're not wrong but I have an impression that this is just a
sarcastic way of telling me this change is not needed. Could you
confirm?

> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  tools/gpio-tools-test.bash | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
> > index abb2f5d..8b4f054 100755
> > --- a/tools/gpio-tools-test.bash
> > +++ b/tools/gpio-tools-test.bash
> > @@ -27,10 +27,10 @@ GPIOSIM_APP_NAME="gpio-tools-test"
> >  MIN_KERNEL_VERSION="5.17.4"
> >  MIN_SHUNIT_VERSION="2.1.8"
> >
> > -# Run the command in $* and fail the test if the command succeeds.
> > +# Run the command in $@ and fail the test if the command succeeds.
> >  assert_fail() {
> > -     $* || return 0
> > -     fail " '$*': command did not fail as expected"
> > +     $@ || return 0
> > +     fail " '$@': command did not fail as expected"
> >  }
> >
> >  # Check if the string in $2 matches against the pattern in $1.
> > @@ -71,7 +71,7 @@ gpiosim_chip() {
> >
> >       mkdir -p $BANKPATH
> >
> > -     for ARG in $*
> > +     for ARG in $@
> >       do
> >               local KEY=$(echo $ARG | cut -d"=" -f1)
> >               local VAL=$(echo $ARG | cut -d"=" -f2)
> > @@ -253,7 +253,7 @@ dut_regex_match() {
> >  }
> >
> >  dut_write() {
> > -     echo $* >&${COPROC[1]}
> > +     echo $@ >&${COPROC[1]}
> >  }
> >
>
> Does echo care?  I realise a linter, and that includes Andy, will complain
> here, but is there a practical difference?
>

The practical difference that comes to mind is passing less arguments
to echo but I don't know if that really matters.

Bart

> Cheers,
> Kent.
>
> >  dut_kill() {
> > @@ -283,7 +283,7 @@ tearDown() {
> >  }
> >
> >  request_release_line() {
> > -     $SOURCE_DIR/gpioget -c $* >/dev/null
> > +     $SOURCE_DIR/gpioget -c $@ >/dev/null
> >  }
> >
> >  #
> >
> > --
> > 2.43.0
> >

Re: [PATCH libgpiod 2/4] tools: tests: use $@ instead of $*

[Kent Gibson]

On Mon, May 27, 2024 at 11:49:09AM +0200, Bartosz Golaszewski wrote:
> On Sat, May 25, 2024 at 3:54 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, May 24, 2024 at 08:03:28PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > $@ does not break up quoted arguments which is what we want in all cases
> > > in the bash test-suite. Use it instead of $*.
> > >
> >
> > I believe it needs to be "$@".  Everywhere.
> >
> > Where do we use quoted arguments/whitespaced parameters?
> > So this is purely about "good" shell?  In that case why stop here - e.g.
> > shellcheck picks up a load more "Double quote to prevent splitting/globbing"
> > and the like.
> >
>
> You're not wrong but I have an impression that this is just a
> sarcastic way of telling me this change is not needed. Could you
> confirm?
>

Me? Sarcastic? ;-)  Well, yes and no, but mainly no.

Strictly speaking, the change is not needed, given the functions in question
are only used internally and we know whitespace is not an issue.

OTOH, I'm fine with this change, but I do think in that case we should fix
everything, to some accepted standard of "good" shell.
I believe Andy suggested the same.  I happened to suggest shellcheck as the
standard as that is what my editor happened to be using.
Happy to go with something else if you have a better alternative.

If you want to apply this series (after fixing the "$@"), I'm happy to patch
that to correct all the other things that shellcheck throws up - there are
lots.

Then if we do happen to make use of whitespace in the future we're good.

Cheers,
Kent.

Re: [PATCH libgpiod 2/4] tools: tests: use $@ instead of $*

[Bartosz Golaszewski]

On Mon, May 27, 2024 at 12:24 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, May 27, 2024 at 11:49:09AM +0200, Bartosz Golaszewski wrote:
> > On Sat, May 25, 2024 at 3:54 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Fri, May 24, 2024 at 08:03:28PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > $@ does not break up quoted arguments which is what we want in all cases
> > > > in the bash test-suite. Use it instead of $*.
> > > >
> > >
> > > I believe it needs to be "$@".  Everywhere.
> > >
> > > Where do we use quoted arguments/whitespaced parameters?
> > > So this is purely about "good" shell?  In that case why stop here - e.g.
> > > shellcheck picks up a load more "Double quote to prevent splitting/globbing"
> > > and the like.
> > >
> >
> > You're not wrong but I have an impression that this is just a
> > sarcastic way of telling me this change is not needed. Could you
> > confirm?
> >
>
> Me? Sarcastic? ;-)  Well, yes and no, but mainly no.
>
> Strictly speaking, the change is not needed, given the functions in question
> are only used internally and we know whitespace is not an issue.
>
> OTOH, I'm fine with this change, but I do think in that case we should fix
> everything, to some accepted standard of "good" shell.
> I believe Andy suggested the same.  I happened to suggest shellcheck as the
> standard as that is what my editor happened to be using.
> Happy to go with something else if you have a better alternative.
>

No, you're right, let's bring it up to shellcheck's standard.

> If you want to apply this series (after fixing the "$@"), I'm happy to patch
> that to correct all the other things that shellcheck throws up - there are
> lots.
>

Sure, knock yourself out. Resend the series with this fixed then.

> Then if we do happen to make use of whitespace in the future we're good.
>
> Cheers,
> Kent.

Thanks,
Bart

[PATCH libgpiod 3/4] tools: tests: remove unneeded ';' in while loops

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We're already breaking the line between while and do so there's no need
for the ';' character.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpio-tools-test.bash | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
index 8b4f054..efd895d 100755
--- a/tools/gpio-tools-test.bash
+++ b/tools/gpio-tools-test.bash
@@ -211,7 +211,7 @@ dut_read_redirect() {
 dut_read() {
 	local LINE
 	lines=()
-	while read -t 0.2 -u ${COPROC[0]} LINE;
+	while read -t 0.2 -u ${COPROC[0]} LINE
 	do
 		if [ -n "$DUT_FIRST_CHAR" ]
 		then
@@ -232,7 +232,7 @@ dut_flush() {
 	lines=()
 	output=
 	unset DUT_FIRST_CHAR
-	while read -t 0 -u ${COPROC[0]} JUNK;
+	while read -t 0 -u ${COPROC[0]} JUNK
 	do
 		read -t 0.1 -u ${COPROC[0]} JUNK || true
 	done

-- 
2.43.0

[PATCH libgpiod 4/4] tools: tests: remove dependency on grep

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We only use grep in one place where we can use the -regex option of find
instead. Switch to using find --regex and drop grep entirely from the
tools test-suite.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpio-tools-test.bash | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/gpio-tools-test.bash b/tools/gpio-tools-test.bash
index efd895d..f910282 100755
--- a/tools/gpio-tools-test.bash
+++ b/tools/gpio-tools-test.bash
@@ -162,7 +162,7 @@ gpiosim_cleanup() {
 		ls $BANKPATH/line* > /dev/null 2>&1
 		if [ "$?" = "0" ]
 		then
-			for LINE in $(find $BANKPATH/ | grep -E "line[0-9]+$")
+			for LINE in $(find $BANKPATH/ -regex ".*line[0-9]+$")
 			do
 				test -e $LINE/hog && rmdir $LINE/hog
 				rmdir $LINE
@@ -3078,7 +3078,6 @@ check_prog() {
 check_prog shunit2
 check_prog modprobe
 check_prog timeout
-check_prog grep
 
 # Check if we're running a kernel at the required version or later
 check_kernel $MIN_KERNEL_VERSION

-- 
2.43.0