[LTP] [PATCH v4 0/4] set -e and bashism fixes

[LTP] [PATCH v4 0/4] set -e and bashism fixes

[Petr Vorel]

changes v3->v4:
* Really fix tst_ansi_color.sh

Petr Vorel (4):
  tst_test.sh: runtest.sh: Remove unused code
  tst_ansi_color.sh: Allow to run with set -e
  tst_test.sh: Fix _tst_cleanup_timer() on set -e
  generate_lvm_runfile.sh: Fix bashism

 lib/newlib_tests/runtest.sh                |  3 ---
 testcases/lib/tst_ansi_color.sh            | 13 +++++++------
 testcases/lib/tst_test.sh                  |  5 +----
 testcases/misc/lvm/generate_lvm_runfile.sh |  5 +++--
 4 files changed, 11 insertions(+), 15 deletions(-)

-- 
2.37.1


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

[LTP] [PATCH v4 1/4] tst_test.sh: runtest.sh: Remove unused code

[Petr Vorel]

This code is in tst_print_colored() (probably forgotten to remove code
from tst_test.sh in a7f613372 when moved to tst_print_colored during
development; later copy wrong code in d132084bc into runtest.sh).

Fixes: a7f613372 ("Colorize test type in the output")
Fixes: d132084bc ("lib: Add script for running tests")

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/newlib_tests/runtest.sh | 3 ---
 testcases/lib/tst_test.sh   | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/lib/newlib_tests/runtest.sh b/lib/newlib_tests/runtest.sh
index f136bcb88..e78b556c5 100755
--- a/lib/newlib_tests/runtest.sh
+++ b/lib/newlib_tests/runtest.sh
@@ -48,9 +48,6 @@ runtest_res()
 	local res="$1"
 	shift
 
-	tst_color_enabled
-	local color=$?
-
 	printf "runtest " >&2
 	tst_print_colored $res "$res: " >&2
 	echo "$@" >&2
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index a35fa2e7b..356af0106 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -112,9 +112,6 @@ tst_res()
 	local res=$1
 	shift
 
-	tst_color_enabled
-	local color=$?
-
 	_tst_inc_res "$res"
 
 	printf "$TST_ID $TST_COUNT " >&2
-- 
2.37.1


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

Re: [LTP] [PATCH v4 1/4] tst_test.sh: runtest.sh: Remove unused code

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 63 bytes --]

Reviewed-by: Li Wang <liwang@redhat.com>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 323 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v4 1/4] tst_test.sh: runtest.sh: Remove unused code

[Martin Doucha]

Hi,
looks good.

Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 08. 08. 22 13:37, Petr Vorel wrote:
> This code is in tst_print_colored() (probably forgotten to remove code
> from tst_test.sh in a7f613372 when moved to tst_print_colored during
> development; later copy wrong code in d132084bc into runtest.sh).
> 
> Fixes: a7f613372 ("Colorize test type in the output")
> Fixes: d132084bc ("lib: Add script for running tests")
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  lib/newlib_tests/runtest.sh | 3 ---
>  testcases/lib/tst_test.sh   | 3 ---
>  2 files changed, 6 deletions(-)
> 
> diff --git a/lib/newlib_tests/runtest.sh b/lib/newlib_tests/runtest.sh
> index f136bcb88..e78b556c5 100755
> --- a/lib/newlib_tests/runtest.sh
> +++ b/lib/newlib_tests/runtest.sh
> @@ -48,9 +48,6 @@ runtest_res()
>  	local res="$1"
>  	shift
>  
> -	tst_color_enabled
> -	local color=$?
> -
>  	printf "runtest " >&2
>  	tst_print_colored $res "$res: " >&2
>  	echo "$@" >&2
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index a35fa2e7b..356af0106 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -112,9 +112,6 @@ tst_res()
>  	local res=$1
>  	shift
>  
> -	tst_color_enabled
> -	local color=$?
> -
>  	_tst_inc_res "$res"
>  
>  	printf "$TST_ID $TST_COUNT " >&2


-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

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

[LTP] [PATCH v4 2/4] tst_ansi_color.sh: Allow to run with set -e

[Petr Vorel]

set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit
code, therefore any && must be turned into || (or if ...; then ..; fi).
Fix hardens tst_res TINFO to be able to be used on scripts with errexit.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
changes v2->v3:
* really fix it.

 testcases/lib/tst_ansi_color.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/testcases/lib/tst_ansi_color.sh b/testcases/lib/tst_ansi_color.sh
index 703df1eb8..517b709d0 100644
--- a/testcases/lib/tst_ansi_color.sh
+++ b/testcases/lib/tst_ansi_color.sh
@@ -24,18 +24,19 @@ tst_flag2color()
 
 tst_color_enabled()
 {
-	[ "$LTP_COLORIZE_OUTPUT" = "n" ] || [ "$LTP_COLORIZE_OUTPUT" = "0" ] && return 0
-	[ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ] && return 1
+	[ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ] || return 1
+	[ "$LTP_COLORIZE_OUTPUT" = "y" -o "$LTP_COLORIZE_OUTPUT" = "1" ] || return 0
 	[ -t 1 ] || return 0
 	return 1
 }
 
 tst_print_colored()
 {
-	tst_color_enabled
-	local color=$?
+	local color=0
 
-	[ "$color" = "1" ] && tst_flag2color "$1"
+	tst_color_enabled || color=$?
+
+	[ "$color" != 1 ] || tst_flag2color "$1"
 	printf "$2"
-	[ "$color" = "1" ] && printf '\033[0m'
+	[ "$color" != 1 ] || printf '\033[0m'
 }
-- 
2.37.1


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

Re: [LTP] [PATCH v4 2/4] tst_ansi_color.sh: Allow to run with set -e

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1886 bytes --]

Hi Petr,

On Mon, Aug 8, 2022 at 7:38 PM Petr Vorel <pvorel@suse.cz> wrote:

> set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit
> code, therefore any && must be turned into || (or if ...; then ..; fi).
> Fix hardens tst_res TINFO to be able to be used on scripts with errexit.
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> changes v2->v3:
> * really fix it.
>
>  testcases/lib/tst_ansi_color.sh | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/testcases/lib/tst_ansi_color.sh
> b/testcases/lib/tst_ansi_color.sh
> index 703df1eb8..517b709d0 100644
> --- a/testcases/lib/tst_ansi_color.sh
> +++ b/testcases/lib/tst_ansi_color.sh
> @@ -24,18 +24,19 @@ tst_flag2color()
>
>  tst_color_enabled()
>  {
> -       [ "$LTP_COLORIZE_OUTPUT" = "n" ] || [ "$LTP_COLORIZE_OUTPUT" = "0"
> ] && return 0
> -       [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1"
> ] && return 1
> +       [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]
> || return 1
> +       [ "$LTP_COLORIZE_OUTPUT" = "y" -o "$LTP_COLORIZE_OUTPUT" = "1" ]
> || return 0
>

This can work but looks a bit strange to read. I personally think
use 'if ...; then ; fi' will be better to understand, because this is a
simple function, no need to go with weird logic for over simplifying:).



>         [ -t 1 ] || return 0
>         return 1
>  }
>
>  tst_print_colored()
>  {
> -       tst_color_enabled
> -       local color=$?
> +       local color=0
>
> -       [ "$color" = "1" ] && tst_flag2color "$1"
> +       tst_color_enabled || color=$?
> +
> +       [ "$color" != 1 ] || tst_flag2color "$1"
>         printf "$2"
> -       [ "$color" = "1" ] && printf '\033[0m'
> +       [ "$color" != 1 ] || printf '\033[0m'
>  }
> --
> 2.37.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 3371 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v4 2/4] tst_ansi_color.sh: Allow to run with set -e

[Petr Vorel]

> Hi Petr,

> On Mon, Aug 8, 2022 at 7:38 PM Petr Vorel <pvorel@suse.cz> wrote:

> > set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit
> > code, therefore any && must be turned into || (or if ...; then ..; fi).
> > Fix hardens tst_res TINFO to be able to be used on scripts with errexit.

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > changes v2->v3:
> > * really fix it.

> >  testcases/lib/tst_ansi_color.sh | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)

> > diff --git a/testcases/lib/tst_ansi_color.sh
> > b/testcases/lib/tst_ansi_color.sh
> > index 703df1eb8..517b709d0 100644
> > --- a/testcases/lib/tst_ansi_color.sh
> > +++ b/testcases/lib/tst_ansi_color.sh
> > @@ -24,18 +24,19 @@ tst_flag2color()

> >  tst_color_enabled()
> >  {
> > -       [ "$LTP_COLORIZE_OUTPUT" = "n" ] || [ "$LTP_COLORIZE_OUTPUT" = "0"
> > ] && return 0
> > -       [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1"
> > ] && return 1
> > +       [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]
> > || return 1
> > +       [ "$LTP_COLORIZE_OUTPUT" = "y" -o "$LTP_COLORIZE_OUTPUT" = "1" ]
> > || return 0

> This can work but looks a bit strange to read. I personally think
> use 'if ...; then ; fi' will be better to understand, because this is a
> simple function, no need to go with weird logic for over simplifying:).

Hi Li,

sure, I can reuse what I posted to as a suggestion to 3rd patch [1],
therefore I'll use it for these two:

if [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]; then
        return 0
fi

if [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ]; then
        return 1
fi

For the latter two I can use 'if ...' as well:

if [ "$color" = 1 ]; then
        tst_flag2color "$1"
fi
printf "$2"
if [ "$color" = 1 ]; then
        printf '\033[0m'
fi

although the original != 1 ] || is IMHO quite readable.

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20220808113756.11582-4-pvorel@suse.cz/

> >         [ -t 1 ] || return 0
> >         return 1
> >  }

> >  tst_print_colored()
> >  {
> > -       tst_color_enabled
> > -       local color=$?
> > +       local color=0

> > -       [ "$color" = "1" ] && tst_flag2color "$1"
> > +       tst_color_enabled || color=$?
> > +
> > +       [ "$color" != 1 ] || tst_flag2color "$1"
> >         printf "$2"
> > -       [ "$color" = "1" ] && printf '\033[0m'
> > +       [ "$color" != 1 ] || printf '\033[0m'
> >  }
> > --
> > 2.37.1


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

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

Re: [LTP] [PATCH v4 2/4] tst_ansi_color.sh: Allow to run with set -e

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 2310 bytes --]

On Tue, Aug 9, 2022 at 8:04 PM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi Petr,
>
> > On Mon, Aug 8, 2022 at 7:38 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > > set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit
> > > code, therefore any && must be turned into || (or if ...; then ..; fi).
> > > Fix hardens tst_res TINFO to be able to be used on scripts with
> errexit.
>
> > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > ---
> > > changes v2->v3:
> > > * really fix it.
>
> > >  testcases/lib/tst_ansi_color.sh | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
>
> > > diff --git a/testcases/lib/tst_ansi_color.sh
> > > b/testcases/lib/tst_ansi_color.sh
> > > index 703df1eb8..517b709d0 100644
> > > --- a/testcases/lib/tst_ansi_color.sh
> > > +++ b/testcases/lib/tst_ansi_color.sh
> > > @@ -24,18 +24,19 @@ tst_flag2color()
>
> > >  tst_color_enabled()
> > >  {
> > > -       [ "$LTP_COLORIZE_OUTPUT" = "n" ] || [ "$LTP_COLORIZE_OUTPUT" =
> "0"
> > > ] && return 0
> > > -       [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" =
> "1"
> > > ] && return 1
> > > +       [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0"
> ]
> > > || return 1
> > > +       [ "$LTP_COLORIZE_OUTPUT" = "y" -o "$LTP_COLORIZE_OUTPUT" = "1"
> ]
> > > || return 0
>
> > This can work but looks a bit strange to read. I personally think
> > use 'if ...; then ; fi' will be better to understand, because this is a
> > simple function, no need to go with weird logic for over simplifying:).
>
> Hi Li,
>
> sure, I can reuse what I posted to as a suggestion to 3rd patch [1],
> therefore I'll use it for these two:
>
> if [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]; then
>         return 0
> fi
>
> if [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ];
> then
>


Should be "-o" but not "||", otherwise looks good to me.



>         return 1
> fi
>
> For the latter two I can use 'if ...' as well:
>
> if [ "$color" = 1 ]; then
>         tst_flag2color "$1"
> fi
> printf "$2"
> if [ "$color" = 1 ]; then
>         printf '\033[0m'
> fi
>
> although the original != 1 ] || is IMHO quite readable.
>

Yeah, but I do not insist on all, just comments for content in the
tst_color_enabled() function.


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 4007 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v4 2/4] tst_ansi_color.sh: Allow to run with set -e

[Petr Vorel]

> On Tue, Aug 9, 2022 at 8:04 PM Petr Vorel <pvorel@suse.cz> wrote:

> > > Hi Petr,

> > > On Mon, Aug 8, 2022 at 7:38 PM Petr Vorel <pvorel@suse.cz> wrote:

> > > > set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit
> > > > code, therefore any && must be turned into || (or if ...; then ..; fi).
> > > > Fix hardens tst_res TINFO to be able to be used on scripts with
> > errexit.

> > > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > > ---
> > > > changes v2->v3:
> > > > * really fix it.

> > > >  testcases/lib/tst_ansi_color.sh | 13 +++++++------
> > > >  1 file changed, 7 insertions(+), 6 deletions(-)

> > > > diff --git a/testcases/lib/tst_ansi_color.sh
> > > > b/testcases/lib/tst_ansi_color.sh
> > > > index 703df1eb8..517b709d0 100644
> > > > --- a/testcases/lib/tst_ansi_color.sh
> > > > +++ b/testcases/lib/tst_ansi_color.sh
> > > > @@ -24,18 +24,19 @@ tst_flag2color()

> > > >  tst_color_enabled()
> > > >  {
> > > > -       [ "$LTP_COLORIZE_OUTPUT" = "n" ] || [ "$LTP_COLORIZE_OUTPUT" =
> > "0"
> > > > ] && return 0
> > > > -       [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" =
> > "1"
> > > > ] && return 1
> > > > +       [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0"
> > ]
> > > > || return 1
> > > > +       [ "$LTP_COLORIZE_OUTPUT" = "y" -o "$LTP_COLORIZE_OUTPUT" = "1"
> > ]
> > > > || return 0

> > > This can work but looks a bit strange to read. I personally think
> > > use 'if ...; then ; fi' will be better to understand, because this is a
> > > simple function, no need to go with weird logic for over simplifying:).

> > Hi Li,

> > sure, I can reuse what I posted to as a suggestion to 3rd patch [1],
> > therefore I'll use it for these two:

> > if [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]; then
> >         return 0
> > fi

> > if [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ];
> > then



> Should be "-o" but not "||", otherwise looks good to me.

Ah, silly error, thanks!

Kind regards,
Petr


> >         return 1
> > fi

> > For the latter two I can use 'if ...' as well:

> > if [ "$color" = 1 ]; then
> >         tst_flag2color "$1"
> > fi
> > printf "$2"
> > if [ "$color" = 1 ]; then
> >         printf '\033[0m'
> > fi

> > although the original != 1 ] || is IMHO quite readable.


> Yeah, but I do not insist on all, just comments for content in the
> tst_color_enabled() function.

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

[LTP] [PATCH v4 3/4] tst_test.sh: Fix _tst_cleanup_timer() on set -e

[Petr Vorel]

If test exits on time (i.e. no timeout) kill in _tst_cleanup_timer()
have nothing to kill therefore following wait exits 143.

set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit code,
fix hardens _tst_cleanup_timer() to be able to be used on scripts with it.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
I can use 'if ...; then ; fi' if you prefer:

	if [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]; then
		return 0
	fi

	if [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ]; then
		return 1
	fi

...
	if [ "$color" = 1 ]; then
		tst_flag2color "$1"
	fi
	printf "$2"
	if [ "$color" = 1 ]; then
		printf '\033[0m'
	fi


Kind regards,
Petr

 testcases/lib/tst_test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 356af0106..1d2bf06cc 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -518,7 +518,7 @@ _tst_cleanup_timer()
 {
 	if [ -n "$_tst_setup_timer_pid" ]; then
 		kill -TERM $_tst_setup_timer_pid 2>/dev/null
-		wait $_tst_setup_timer_pid 2>/dev/null
+		wait $_tst_setup_timer_pid 2>/dev/null || true
 	fi
 }
 
-- 
2.37.1


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

Re: [LTP] [PATCH v4 3/4] tst_test.sh: Fix _tst_cleanup_timer() on set -e

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1072 bytes --]

On Mon, Aug 8, 2022 at 7:39 PM Petr Vorel <pvorel@suse.cz> wrote:

> If test exits on time (i.e. no timeout) kill in _tst_cleanup_timer()
> have nothing to kill therefore following wait exits 143.
>
> set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit code,
> fix hardens _tst_cleanup_timer() to be able to be used on scripts with it.
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> I can use 'if ...; then ; fi' if you prefer:
>
>         if [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0"
> ]; then
>                 return 0
>         fi
>
>         if [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" =
> "1" ]; then
>                 return 1
>         fi
>
> ...
>         if [ "$color" = 1 ]; then
>                 tst_flag2color "$1"
>         fi
>         printf "$2"
>         if [ "$color" = 1 ]; then
>                 printf '\033[0m'
>         fi
>

These ^ clarifications should be added in patch 2/4, right?

Anyway, this one looks good.
Reviewed-by: Li Wang <liwang@redhat.com>


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2153 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v4 3/4] tst_test.sh: Fix _tst_cleanup_timer() on set -e

[Petr Vorel]

Hi Li,

> These ^ clarifications should be added in patch 2/4, right?
Yes, put it there.

> Anyway, this one looks good.
> Reviewed-by: Li Wang <liwang@redhat.com>
Thanks for your time!

Kind regards,
Petr

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

Re: [LTP] [PATCH v4 3/4] tst_test.sh: Fix _tst_cleanup_timer() on set -e

[Martin Doucha]

Hi,
a comment explaining why this is needed would be nice, otherwise looks
good. For patches 2 and 3:

Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 08. 08. 22 13:37, Petr Vorel wrote:
> If test exits on time (i.e. no timeout) kill in _tst_cleanup_timer()
> have nothing to kill therefore following wait exits 143.
> 
> set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit code,
> fix hardens _tst_cleanup_timer() to be able to be used on scripts with it.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> I can use 'if ...; then ; fi' if you prefer:
> 
> 	if [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]; then
> 		return 0
> 	fi
> 
> 	if [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ]; then
> 		return 1
> 	fi
> 
> ...
> 	if [ "$color" = 1 ]; then
> 		tst_flag2color "$1"
> 	fi
> 	printf "$2"
> 	if [ "$color" = 1 ]; then
> 		printf '\033[0m'
> 	fi
> 
> 
> Kind regards,
> Petr
> 
>  testcases/lib/tst_test.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 356af0106..1d2bf06cc 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -518,7 +518,7 @@ _tst_cleanup_timer()
>  {
>  	if [ -n "$_tst_setup_timer_pid" ]; then
>  		kill -TERM $_tst_setup_timer_pid 2>/dev/null
> -		wait $_tst_setup_timer_pid 2>/dev/null
> +		wait $_tst_setup_timer_pid 2>/dev/null || true
>  	fi
>  }
>  


-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

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

Re: [LTP] [PATCH v4 3/4] tst_test.sh: Fix _tst_cleanup_timer() on set -e

[Petr Vorel]

Hi Martin,

> Hi,
> a comment explaining why this is needed would be nice, otherwise looks
> good. For patches 2 and 3:

> Reviewed-by: Martin Doucha <mdoucha@suse.cz>

> On 08. 08. 22 13:37, Petr Vorel wrote:
> > If test exits on time (i.e. no timeout) kill in _tst_cleanup_timer()
> > have nothing to kill therefore following wait exits 143.
I thought this is the explanation. Or would you prefer anything else to add?

Kind regards,
Petr

> > set -e (or #!/bin/sh -e or set -o errexit) quits on any non-zero exit code,
> > fix hardens _tst_cleanup_timer() to be able to be used on scripts with it.

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > I can use 'if ...; then ; fi' if you prefer:

> > 	if [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]; then
> > 		return 0
> > 	fi

> > 	if [ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ]; then
> > 		return 1
> > 	fi

> > ...
> > 	if [ "$color" = 1 ]; then
> > 		tst_flag2color "$1"
> > 	fi
> > 	printf "$2"
> > 	if [ "$color" = 1 ]; then
> > 		printf '\033[0m'
> > 	fi


> > Kind regards,
> > Petr

> >  testcases/lib/tst_test.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> > index 356af0106..1d2bf06cc 100644
> > --- a/testcases/lib/tst_test.sh
> > +++ b/testcases/lib/tst_test.sh
> > @@ -518,7 +518,7 @@ _tst_cleanup_timer()
> >  {
> >  	if [ -n "$_tst_setup_timer_pid" ]; then
> >  		kill -TERM $_tst_setup_timer_pid 2>/dev/null
> > -		wait $_tst_setup_timer_pid 2>/dev/null
> > +		wait $_tst_setup_timer_pid 2>/dev/null || true
> >  	fi
> >  }

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

Re: [LTP] [PATCH v4 3/4] tst_test.sh: Fix _tst_cleanup_timer() on set -e

[Martin Doucha]

On 10. 08. 22 18:29, Petr Vorel wrote:
> Hi Martin,
> 
>> Hi,
>> a comment explaining why this is needed would be nice, otherwise looks
>> good. For patches 2 and 3:
> 
>> Reviewed-by: Martin Doucha <mdoucha@suse.cz>
> 
>> On 08. 08. 22 13:37, Petr Vorel wrote:
>>> If test exits on time (i.e. no timeout) kill in _tst_cleanup_timer()
>>> have nothing to kill therefore following wait exits 143.
> I thought this is the explanation. Or would you prefer anything else to add?

The commit message will get buried deep in Git history. It's better to
comment this in the code because it's not obvious why the "|| true" is
needed after "wait ..."

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

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

Re: [LTP] [PATCH v4 3/4] tst_test.sh: Fix _tst_cleanup_timer() on set -e

[Petr Vorel]

> On 10. 08. 22 18:29, Petr Vorel wrote:
> > Hi Martin,

> >> Hi,
> >> a comment explaining why this is needed would be nice, otherwise looks
> >> good. For patches 2 and 3:

> >> Reviewed-by: Martin Doucha <mdoucha@suse.cz>

> >> On 08. 08. 22 13:37, Petr Vorel wrote:
> >>> If test exits on time (i.e. no timeout) kill in _tst_cleanup_timer()
> >>> have nothing to kill therefore following wait exits 143.
> > I thought this is the explanation. Or would you prefer anything else to add?

> The commit message will get buried deep in Git history. It's better to
> comment this in the code because it's not obvious why the "|| true" is
> needed after "wait ..."
Ah, didn't get "comment" means comment in the code. Makes sense, thx!

Kind regards,
Petr

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

Re: [LTP] [PATCH v4 3/4] tst_test.sh: Fix _tst_cleanup_timer() on set -e

[Petr Vorel]

Hi all,

FYI merged.

Kind regards,
Petr

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

[LTP] [PATCH v4 4/4] generate_lvm_runfile.sh: Fix bashism

[Petr Vorel]

ERR is not on dash (tested on 0.5.11).

using -e (i.e. implying 'set -e') is required to exit on any non zero
exit code. It requires 2 previous fixes.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/misc/lvm/generate_lvm_runfile.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/testcases/misc/lvm/generate_lvm_runfile.sh b/testcases/misc/lvm/generate_lvm_runfile.sh
index 72b286a69..aba706c64 100755
--- a/testcases/misc/lvm/generate_lvm_runfile.sh
+++ b/testcases/misc/lvm/generate_lvm_runfile.sh
@@ -1,6 +1,7 @@
-#!/bin/sh
+#!/bin/sh -e
 # SPDX-License-Identifier: GPL-2.0-or-later
 # Copyright (c) 2020 SUSE LLC <mdoucha@suse.cz>
+# Copyright (c) Linux Test Project, 2022
 #
 # Generate LTP runfile for LVM tests (runtest/lvm.local)
 
@@ -13,7 +14,7 @@ LVM_TMPDIR="$LVM_DIR/ltp/growfiles"
 
 generate_runfile()
 {
-	trap 'tst_brk TBROK "Cannot create LVM runfile"' ERR
+	trap '[ $? -eq 0 ] || tst_brk TBROK "Cannot create LVM runfile"' EXIT
 	INFILE="$LTPROOT/testcases/data/lvm/runfile.tpl"
 	OUTFILE="$LTPROOT/runtest/lvm.local"
 	FS_LIST=`tst_supported_fs`
-- 
2.37.1


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

Re: [LTP] [PATCH v4 4/4] generate_lvm_runfile.sh: Fix bashism

[Li Wang]

[-- Attachment #1.1: Type: text/plain, Size: 63 bytes --]

Reviewed-by: Li Wang <liwang@redhat.com>

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 361 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH v4 4/4] generate_lvm_runfile.sh: Fix bashism

[Martin Doucha]

Hi,
I've tested the patchset and the generator script now work correctly.

Reviewed-by: Martin Doucha <mdoucha@suse.cz>

On 08. 08. 22 13:37, Petr Vorel wrote:
> ERR is not on dash (tested on 0.5.11).
> 
> using -e (i.e. implying 'set -e') is required to exit on any non zero
> exit code. It requires 2 previous fixes.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  testcases/misc/lvm/generate_lvm_runfile.sh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/misc/lvm/generate_lvm_runfile.sh b/testcases/misc/lvm/generate_lvm_runfile.sh
> index 72b286a69..aba706c64 100755
> --- a/testcases/misc/lvm/generate_lvm_runfile.sh
> +++ b/testcases/misc/lvm/generate_lvm_runfile.sh
> @@ -1,6 +1,7 @@
> -#!/bin/sh
> +#!/bin/sh -e
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  # Copyright (c) 2020 SUSE LLC <mdoucha@suse.cz>
> +# Copyright (c) Linux Test Project, 2022
>  #
>  # Generate LTP runfile for LVM tests (runtest/lvm.local)
>  
> @@ -13,7 +14,7 @@ LVM_TMPDIR="$LVM_DIR/ltp/growfiles"
>  
>  generate_runfile()
>  {
> -	trap 'tst_brk TBROK "Cannot create LVM runfile"' ERR
> +	trap '[ $? -eq 0 ] || tst_brk TBROK "Cannot create LVM runfile"' EXIT
>  	INFILE="$LTPROOT/testcases/data/lvm/runfile.tpl"
>  	OUTFILE="$LTPROOT/runtest/lvm.local"
>  	FS_LIST=`tst_supported_fs`


-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

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

Re: [LTP] [PATCH v4 4/4] generate_lvm_runfile.sh: Fix bashism

[Petr Vorel]

Hi Martin,

> Hi,
> I've tested the patchset and the generator script now work correctly.

Thanks! I'll wait unless you suggest anything on 3rd commit and merge with this
style change below. FYI I tested various setup:

for i in y n 0 1 ""; do
	echo "== '$i' =="
	LTP_COLORIZE_OUTPUT=$i PATH="/opt/ltp/testcases/bin:$PATH" $f
	rc=$?
	echo "'$i': $rc"
	[ $rc -eq 0 ] || break
done

I also added patch for this test
https://lore.kernel.org/ltp/20220808122716.18556-1-pvorel@suse.cz/

Kind regards,
Petr

> Reviewed-by: Martin Doucha <mdoucha@suse.cz>

diff --git testcases/lib/tst_ansi_color.sh testcases/lib/tst_ansi_color.sh
index 517b709d0..04e120cf7 100644
--- testcases/lib/tst_ansi_color.sh
+++ testcases/lib/tst_ansi_color.sh
@@ -24,9 +24,16 @@ tst_flag2color()
 
 tst_color_enabled()
 {
-	[ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ] || return 1
-	[ "$LTP_COLORIZE_OUTPUT" = "y" -o "$LTP_COLORIZE_OUTPUT" = "1" ] || return 0
+	if [ "$LTP_COLORIZE_OUTPUT" = "n" -o "$LTP_COLORIZE_OUTPUT" = "0" ]; then
+		return 0
+	fi
+
+	if [ "$LTP_COLORIZE_OUTPUT" = "y" -o "$LTP_COLORIZE_OUTPUT" = "1" ]; then
+		return 0
+	fi
+
 	[ -t 1 ] || return 0
+
 	return 1
 }
 

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