| Message ID | 20220808113756.11582-3-pvorel@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | set -e and bashism fixes | expand |
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 > >
> 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
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.
> 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.
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' }
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(-)