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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, May 27, 2024 at 3:20 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Mon, May 27, 2024 at 08:57:32PM +0800, Kent Gibson wrote:
> > On Mon, May 27, 2024 at 02:51:52PM +0200, Bartosz Golaszewski wrote:
> > > On Mon, May 27, 2024 at 2:44 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > > >
> > > > On Mon, May 27, 2024 at 02:02:34PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > > > >
> > > > > $@ does not break up quoted arguments which is what we want in all cases
> > > > > in the bash test-suite. Use it instead of $*. While at it: prevent
> > > > > globbing with double quotes but allow variable expansion.
> > > > >
> > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > > > > ---
> > > > >  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..dde26b4 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"
> > > > >  }
> > > > >
> > > >
> > > > Ironically, shellcheck doesn't like the '$@' in the fail string[1], so you
> > > > should use $* there.
> > > >
> > > > It also doesn't like looping on find results in patch 4[2], though that
> > > > is not related to your change, so leave it and I'll fix it later?
> > > >
> > >
> > > What does it want here? This looks correct to me? Should we do "$(find...)"?
> > >
> >
> > Refer to the referenced link - it is worried about filenames containing
> > whitespace.
> > Not sure what the best option is here - I am only just looking into it...
> >
>
> How about using this for the cleanup:
>
>                 echo 0 > "$DEVPATH/live"
>                 find "$DEVPATH" -type d -name hog -exec rmdir '{}' '+'
>                 find "$DEVPATH" -type d -name "line*" -exec rmdir '{}' '+'
>                 find "$DEVPATH" -type d -name "bank*" -exec rmdir '{}' '+'
>                 rmdir "$DEVPATH"
>
> It is a bit less subtle, but that works for me.
>

Looks good and works fine. I'll use it, thanks a lot!

Bart