| Message ID | 20220808113756.11582-4-pvorel@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | set -e and bashism fixes | expand |
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>
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
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 > } >
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 > > }
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 ..."
> 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
Hi all, FYI merged. Kind regards, Petr
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 }
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(-)