* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
@ 2021-09-02 10:37 Petr Vorel
2021-09-02 10:37 ` [LTP] [PATCH 1/4] doc: Mention make check Petr Vorel
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 10:37 UTC (permalink / raw)
To: ltp
Hi,
checkbashisms.pl has problem with type. Although it's in POSIX [1] even
in old one from 2004 [2] and it's supported by all common shells (i.e.
bash, zsh, dash, busybox sh, mksh; even in ksh; maybe just csh does not
support it) checkbashisms.pl complains about it:
$ make check-tst_test.sh
CHECK testcases/lib/tst_test.sh
possible bashism in tst_test.sh line 33 (type):
if type $TST_CLEANUP >/dev/null 2>/dev/null; then
possible bashism in tst_test.sh line 694 (type):
if type $TST_SETUP >/dev/null 2>/dev/null; then
possible bashism in tst_test.sh line 726 (type):
if type ${TST_TESTFUNC}1 > /dev/null 2>&1; then
make: [../../include/mk/rules.mk:58: check-tst_test.sh] Error 1 (ignored)
Should I report it to Debian (the upstream)? Or at least ask for way to
suppress the warning?
Kind regards,
Petr
[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/type.html#tag_20_136
[2] https://pubs.opengroup.org/onlinepubs/000095399/utilities/type.html
Petr Vorel (4):
doc: Mention make check
Vendor checkbashisms.pl version 2.20.5
rules.mk: Add checkbashisms to 'make check' for *.sh
doc: Update for vendored checkbashisms.pl
doc/c-test-tutorial-simple.txt | 21 +-
doc/maintainer-patch-review-checklist.txt | 3 +-
doc/test-writing-guidelines.txt | 18 +-
include/mk/env_post.mk | 2 +
include/mk/generic_leaf_target.inc | 2 +-
include/mk/generic_trunk_target.inc | 2 +-
include/mk/rules.mk | 9 +
scripts/checkbashisms.pl | 816 ++++++++++++++++++++++
8 files changed, 851 insertions(+), 22 deletions(-)
create mode 100755 scripts/checkbashisms.pl
--
2.33.0
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 1/4] doc: Mention make check
2021-09-02 10:37 [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
@ 2021-09-02 10:37 ` Petr Vorel
2021-09-02 13:17 ` Cyril Hrubis
2021-09-02 10:37 ` [LTP] [PATCH 2/4] Vendor checkbashisms.pl version 2.20.5 Petr Vorel
` (4 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 10:37 UTC (permalink / raw)
To: ltp
and our version of checkpatch.pl instead of suggesting to use upstream
on remaining places (just to be consistent).
Fixes: 4c1e61cc7 ("docs: rm explicit reference to checkpatch.pl")
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
doc/c-test-tutorial-simple.txt | 21 ++++++++++-----------
doc/test-writing-guidelines.txt | 10 +++++-----
2 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/doc/c-test-tutorial-simple.txt b/doc/c-test-tutorial-simple.txt
index aa6d19f8d..bacf2f1d9 100644
--- a/doc/c-test-tutorial-simple.txt
+++ b/doc/c-test-tutorial-simple.txt
@@ -228,12 +228,12 @@ the lines starting with a +++.
--------------------------------------------------------------------------------
statvfs01 statvfs01
statvfs02 statvfs02
-
+
+statx01 statx01
+
stime01 stime01
stime02 stime02
-
+
--------------------------------------------------------------------------------
The +runtest+ files are in a two column format. The first column is the test
@@ -306,12 +306,11 @@ Wiki]).
Is your +.gitignore+ correct?
-3.3 Run checkpatch.pl on the source file
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+3.3 Run make check
+~~~~~~~~~~~~~~~~~~
-The LTP follows the Linux style guidelines where possible. Check what happens
-if you run +kernel/linux/scripts/checkpatch.pl --no-tree -f statx01.c+ and
-correct any style issues.
+Check coding style with `make check`
+ (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#21-c-coding-style[C coding style])
3.4 Install the LTP and run the test with runtest
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -767,7 +766,7 @@ failing due to some faulty logic.
6.1 What is wrong with the switch statement?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Were you paying attention? Also see the output of +checkpatch.pl+.
+Were you paying attention? Also see the output of +make check+.
6.2 Test a feature unique to statx
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -795,9 +794,9 @@ of fallback logic in the build system. We can now get our test ready for
submission.
The first thing you need to do before considering submitting your test is run
-+scripts/checkpatch.pl --no-tree -f+ on +statx01.c+. Again, we use the kernel
-style guidelines where possible. Next you should create a new branch, this
-will allow you to reshape your commit history without fear.
++make check-statx01+ or + make check+ in the test's directory. Again, we use
+the kernel style guidelines where possible. Next you should create a new
+branch, this will allow you to reshape your commit history without fear.
After that we have the pleasure of doing an interactive 'rebase' to clean up
our commit history. In its current form the test only really needs a single
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 12fe4922a..8053f0cb0 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -125,13 +125,13 @@ LTP adopted Linux kernel coding style. If you aren't familiar with its rules
locate 'linux/Documentation/CodingStyle' in the kernel sources and read it,
it's a well written introduction.
-There is also
+Run `make check` in the test's directory and/or use `make check-$TCID`,
+it uses (among other checks) our vendored version of
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
-script from kernel git tree which can be used to check your patches before the
-submission. Please use reasonably recent one.
+script from kernel git tree.
-NOTE: If checkpatch.pl does not report any problems, the code still may be wrong
- as the tool only looks for common mistakes.
+NOTE: If `make check` does not report any problems, the code still may be wrong
+ as all tools used for checking only look for common mistakes.
2.2 Shell coding style
^^^^^^^^^^^^^^^^^^^^^^
--
2.33.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [LTP] [PATCH 2/4] Vendor checkbashisms.pl version 2.20.5
2021-09-02 10:37 [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
2021-09-02 10:37 ` [LTP] [PATCH 1/4] doc: Mention make check Petr Vorel
@ 2021-09-02 10:37 ` Petr Vorel
2021-09-02 13:25 ` Cyril Hrubis
2021-09-02 10:37 ` [LTP] [PATCH 3/4] rules.mk: Add checkbashisms to 'make check' for *.sh Petr Vorel
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 10:37 UTC (permalink / raw)
To: ltp
From https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl
(updated version in the script)
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
scripts/checkbashisms.pl | 816 +++++++++++++++++++++++++++++++++++++++
1 file changed, 816 insertions(+)
create mode 100755 scripts/checkbashisms.pl
diff --git a/scripts/checkbashisms.pl b/scripts/checkbashisms.pl
new file mode 100755
index 000000000..ba417c993
--- /dev/null
+++ b/scripts/checkbashisms.pl
@@ -0,0 +1,816 @@
+#!/usr/bin/perl
+
+# This script is essentially copied from /usr/share/lintian/checks/scripts,
+# which is:
+# Copyright (C) 1998 Richard Braakman
+# Copyright (C) 2002 Josip Rodin
+# This version is
+# Copyright (C) 2003 Julian Gilbey
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+use strict;
+use warnings;
+use Getopt::Long qw(:config bundling permute no_getopt_compat);
+use File::Temp qw/tempfile/;
+
+sub init_hashes;
+
+(my $progname = $0) =~ s|.*/||;
+
+my $usage = <<"EOF";
+Usage: $progname [-n] [-f] [-x] [-e] script ...
+ or: $progname --help
+ or: $progname --version
+This script performs basic checks for the presence of bashisms
+in /bin/sh scripts and the lack of bashisms in /bin/bash ones.
+EOF
+
+my $version = <<"EOF";
+This is $progname, from the Debian devscripts package, version 2.20.5
+This code is copyright 2003 by Julian Gilbey <jdg\@debian.org>,
+based on original code which is copyright 1998 by Richard Braakman
+and copyright 2002 by Josip Rodin.
+This program comes with ABSOLUTELY NO WARRANTY.
+You are free to redistribute this code under the terms of the
+GNU General Public License, version 2, or (at your option) any later version.
+EOF
+
+my ($opt_echo, $opt_force, $opt_extra, $opt_posix, $opt_early_fail);
+my ($opt_help, $opt_version);
+my @filenames;
+
+# Detect if STDIN is a pipe
+if (scalar(@ARGV) == 0 && (-p STDIN or -f STDIN)) {
+ push(@ARGV, '-');
+}
+
+##
+## handle command-line options
+##
+$opt_help = 1 if int(@ARGV) == 0;
+
+GetOptions(
+ "help|h" => \$opt_help,
+ "version|v" => \$opt_version,
+ "newline|n" => \$opt_echo,
+ "force|f" => \$opt_force,
+ "extra|x" => \$opt_extra,
+ "posix|p" => \$opt_posix,
+ "early-fail|e" => \$opt_early_fail,
+ )
+ or die
+"Usage: $progname [options] filelist\nRun $progname --help for more details\n";
+
+if ($opt_help) { print $usage; exit 0; }
+if ($opt_version) { print $version; exit 0; }
+
+$opt_echo = 1 if $opt_posix;
+
+my $mode = 0;
+my $issues = 0;
+my $status = 0;
+my $makefile = 0;
+my (%bashisms, %string_bashisms, %singlequote_bashisms);
+
+my $LEADIN
+ = qr'(?:(?:^|[`&;(|{])\s*|(?:(?:if|elif|while)(?:\s+!)?|then|do|shell)\s+)';
+init_hashes;
+
+my @bashisms_keys = sort keys %bashisms;
+my @string_bashisms_keys = sort keys %string_bashisms;
+my @singlequote_bashisms_keys = sort keys %singlequote_bashisms;
+
+foreach my $filename (@ARGV) {
+ my $check_lines_count = -1;
+
+ my $display_filename = $filename;
+
+ if ($filename eq '-') {
+ my $tmp_fh;
+ ($tmp_fh, $filename)
+ = tempfile("chkbashisms_tmp.XXXX", TMPDIR => 1, UNLINK => 1);
+ while (my $line = <STDIN>) {
+ print $tmp_fh $line;
+ }
+ close($tmp_fh);
+ $display_filename = "(stdin)";
+ }
+
+ if (!$opt_force) {
+ $check_lines_count = script_is_evil_and_wrong($filename);
+ }
+
+ if ($check_lines_count == 0 or $check_lines_count == 1) {
+ warn
+"script $display_filename does not appear to be a /bin/sh script; skipping\n";
+ next;
+ }
+
+ if ($check_lines_count != -1) {
+ warn
+"script $display_filename appears to be a shell wrapper; only checking the first "
+ . "$check_lines_count lines\n";
+ }
+
+ unless (open C, '<', $filename) {
+ warn "cannot open script $display_filename for reading: $!\n";
+ $status |= 2;
+ next;
+ }
+
+ $issues = 0;
+ $mode = 0;
+ my $cat_string = "";
+ my $cat_indented = 0;
+ my $quote_string = "";
+ my $last_continued = 0;
+ my $continued = 0;
+ my $found_rules = 0;
+ my $buffered_orig_line = "";
+ my $buffered_line = "";
+ my %start_lines;
+
+ while (<C>) {
+ next unless ($check_lines_count == -1 or $. <= $check_lines_count);
+
+ if ($. == 1) { # This should be an interpreter line
+ if (m,^\#!\s*(?:\S+/env\s+)?(\S+),) {
+ my $interpreter = $1;
+
+ if ($interpreter =~ m,(?:^|/)make$,) {
+ init_hashes if !$makefile++;
+ $makefile = 1;
+ } else {
+ init_hashes if $makefile--;
+ $makefile = 0;
+ }
+ next if $opt_force;
+
+ if ($interpreter =~ m,(?:^|/)bash$,) {
+ $mode = 1;
+ } elsif ($interpreter !~ m,(?:^|/)(sh|dash|posh)$,) {
+### ksh/zsh?
+ warn
+"script $display_filename does not appear to be a /bin/sh script; skipping\n";
+ $status |= 2;
+ last;
+ }
+ } else {
+ warn
+"script $display_filename does not appear to have a \#! interpreter line;\nyou may get strange results\n";
+ }
+ }
+
+ chomp;
+ my $orig_line = $_;
+
+ # We want to remove end-of-line comments, so need to skip
+ # comments that appear inside balanced pairs
+ # of single or double quotes
+
+ # Remove comments in the "quoted" part of a line that starts
+ # in a quoted block? The problem is that we have no idea
+ # whether the program interpreting the block treats the
+ # quote character as part of the comment or as a quote
+ # terminator. We err on the side of caution and assume it
+ # will be treated as part of the comment.
+ # s/^(?:.*?[^\\])?$quote_string(.*)$/$1/ if $quote_string ne "";
+
+ # skip comment lines
+ if ( m,^\s*\#,
+ && $quote_string eq ''
+ && $buffered_line eq ''
+ && $cat_string eq '') {
+ next;
+ }
+
+ # Remove quoted strings so we can more easily ignore comments
+ # inside them
+ s/(^|[^\\](?:\\\\)*)\'(?:\\.|[^\\\'])+\'/$1''/g;
+ s/(^|[^\\](?:\\\\)*)\"(?:\\.|[^\\\"])+\"/$1""/g;
+
+ # If inside a quoted string, remove everything before the quote
+ s/^.+?\'//
+ if ($quote_string eq "'");
+ s/^.+?[^\\]\"//
+ if ($quote_string eq '"');
+
+ # If the remaining string contains what looks like a comment,
+ # eat it. In either case, swap the unmodified script line
+ # back in for processing.
+ if (m/(?:^|[^[\\])[\s\&;\(\)](\#.*$)/) {
+ $_ = $orig_line;
+ s/\Q$1\E//; # eat comments
+ } else {
+ $_ = $orig_line;
+ }
+
+ # Handle line continuation
+ if (!$makefile && $cat_string eq '' && m/\\$/) {
+ chop;
+ $buffered_line .= $_;
+ $buffered_orig_line .= $orig_line . "\n";
+ next;
+ }
+
+ if ($buffered_line ne '') {
+ $_ = $buffered_line . $_;
+ $orig_line = $buffered_orig_line . $orig_line;
+ $buffered_line = '';
+ $buffered_orig_line = '';
+ }
+
+ if ($makefile) {
+ $last_continued = $continued;
+ if (/[^\\]\\$/) {
+ $continued = 1;
+ } else {
+ $continued = 0;
+ }
+
+ # Don't match lines that look like a rule if we're in a
+ # continuation line before the start of the rules
+ if (/^[\w%-]+:+\s.*?;?(.*)$/
+ and !($last_continued and !$found_rules)) {
+ $found_rules = 1;
+ $_ = $1 if $1;
+ }
+
+ last
+ if m%^\s*(override\s|export\s)?\s*SHELL\s*:?=\s*(/bin/)?bash\s*%;
+
+ # Remove "simple" target names
+ s/^[\w%.-]+(?:\s+[\w%.-]+)*::?//;
+ s/^\t//;
+ s/(?<!\$)\$\((\w+)\)/\${$1}/g;
+ s/(\$){2}/$1/g;
+ s/^[\s\t]*[@-]{1,2}//;
+ }
+
+ if (
+ $cat_string ne ""
+ && (m/^\Q$cat_string\E$/
+ || ($cat_indented && m/^\t*\Q$cat_string\E$/))
+ ) {
+ $cat_string = "";
+ next;
+ }
+ my $within_another_shell = 0;
+ if (m,(^|\s+)((/usr)?/bin/)?((b|d)?a|k|z|t?c)sh\s+-c\s*.+,) {
+ $within_another_shell = 1;
+ }
+ # if cat_string is set, we are in a HERE document and need not
+ # check for things
+ if ($cat_string eq "" and !$within_another_shell) {
+ my $found = 0;
+ my $match = '';
+ my $explanation = '';
+ my $line = $_;
+
+ # Remove "" / '' as they clearly aren't quoted strings
+ # and not considering them makes the matching easier
+ $line =~ s/(^|[^\\])(\'\')+/$1/g;
+ $line =~ s/(^|[^\\])(\"\")+/$1/g;
+
+ if ($quote_string ne "") {
+ my $otherquote = ($quote_string eq "\"" ? "\'" : "\"");
+ # Inside a quoted block
+ if ($line =~ /(?:^|^.*?[^\\])$quote_string(.*)$/) {
+ my $rest = $1;
+ my $templine = $line;
+
+ # Remove quoted strings delimited with $otherquote
+ $templine
+ =~ s/(^|[^\\])$otherquote[^$quote_string]*?[^\\]$otherquote/$1/g;
+ # Remove quotes that are themselves quoted
+ # "a'b"
+ $templine
+ =~ s/(^|[^\\])$otherquote.*?$quote_string.*?[^\\]$otherquote/$1/g;
+ # "\""
+ $templine
+ =~ s/(^|[^\\])$quote_string\\$quote_string$quote_string/$1/g;
+
+ # After all that, were there still any quotes left?
+ my $count = () = $templine =~ /(^|[^\\])$quote_string/g;
+ next if $count == 0;
+
+ $count = () = $rest =~ /(^|[^\\])$quote_string/g;
+ if ($count % 2 == 0) {
+ # Quoted block ends on this line
+ # Ignore everything before the closing quote
+ $line = $rest || '';
+ $quote_string = "";
+ } else {
+ next;
+ }
+ } else {
+ # Still inside the quoted block, skip this line
+ next;
+ }
+ }
+
+ # Check even if we removed the end of a quoted block
+ # in the previous check, as a single line can end one
+ # block and begin another
+ if ($quote_string eq "") {
+ # Possible start of a quoted block
+ for my $quote ("\"", "\'") {
+ my $templine = $line;
+ my $otherquote = ($quote eq "\"" ? "\'" : "\"");
+
+ # Remove balanced quotes and their content
+ while (1) {
+ my ($length_single, $length_double) = (0, 0);
+
+ # Determine which one would match first:
+ if ($templine
+ =~ m/(^.+?(?:^|[^\\\"](?:\\\\)*)\')[^\']*\'/) {
+ $length_single = length($1);
+ }
+ if ($templine
+ =~ m/(^.*?(?:^|[^\\\'](?:\\\\)*)\")(?:\\.|[^\\\"])+\"/
+ ) {
+ $length_double = length($1);
+ }
+
+ # Now simplify accordingly (shorter is preferred):
+ if (
+ $length_single != 0
+ && ( $length_single < $length_double
+ || $length_double == 0)
+ ) {
+ $templine =~ s/(^|[^\\\"](?:\\\\)*)\'[^\']*\'/$1/;
+ } elsif ($length_double != 0) {
+ $templine
+ =~ s/(^|[^\\\'](?:\\\\)*)\"(?:\\.|[^\\\"])+\"/$1/;
+ } else {
+ last;
+ }
+ }
+
+ # Don't flag quotes that are themselves quoted
+ # "a'b"
+ $templine =~ s/$otherquote.*?$quote.*?$otherquote//g;
+ # "\""
+ $templine =~ s/(^|[^\\])$quote\\$quote$quote/$1/g;
+ # \' or \"
+ $templine =~ s/\\[\'\"]//g;
+ my $count = () = $templine =~ /(^|(?!\\))$quote/g;
+
+ # If there's an odd number of non-escaped
+ # quotes in the line it's almost certainly the
+ # start of a quoted block.
+ if ($count % 2 == 1) {
+ $quote_string = $quote;
+ $start_lines{'quote_string'} = $.;
+ $line =~ s/^(.*)$quote.*$/$1/;
+ last;
+ }
+ }
+ }
+
+ # since this test is ugly, I have to do it by itself
+ # detect source (.) trying to pass args to the command it runs
+ # The first expression weeds out '. "foo bar"'
+ if ( not $found
+ and not
+m/$LEADIN\.\s+(\"[^\"]+\"|\'[^\']+\'|\$\([^)]+\)+(?:\/[^\s;]+)?)\s*(\&|\||\d?>|<|;|\Z)/o
+ and m/$LEADIN(\.\s+[^\s;\`:]+\s+([^\s;]+))/o) {
+ if ($2 =~ /^(\&|\||\d?>|<)/) {
+ # everything is ok
+ ;
+ } else {
+ $found = 1;
+ $match = $1;
+ $explanation = "sourced script with arguments";
+ output_explanation($display_filename, $orig_line,
+ $explanation);
+ }
+ }
+
+ # Remove "quoted quotes". They're likely to be inside
+ # another pair of quotes; we're not interested in
+ # them for their own sake and removing them makes finding
+ # the limits of the outer pair far easier.
+ $line =~ s/(^|[^\\\'\"])\"\'\"/$1/g;
+ $line =~ s/(^|[^\\\'\"])\'\"\'/$1/g;
+
+ foreach my $re (@singlequote_bashisms_keys) {
+ my $expl = $singlequote_bashisms{$re};
+ if ($line =~ m/($re)/) {
+ $found = 1;
+ $match = $1;
+ $explanation = $expl;
+ output_explanation($display_filename, $orig_line,
+ $explanation);
+ }
+ }
+
+ my $re = '(?<![\$\\\])\$\'[^\']+\'';
+ if ($line =~ m/(.*)($re)/o) {
+ my $count = () = $1 =~ /(^|[^\\])\'/g;
+ if ($count % 2 == 0) {
+ output_explanation($display_filename, $orig_line,
+ q<$'...' should be "$(printf '...')">);
+ }
+ }
+
+ # $cat_line contains the version of the line we'll check
+ # for heredoc delimiters later. Initially, remove any
+ # spaces between << and the delimiter to make the following
+ # updates to $cat_line easier. However, don't remove the
+ # spaces if the delimiter starts with a -, as that changes
+ # how the delimiter is searched.
+ my $cat_line = $line;
+ $cat_line =~ s/(<\<-?)\s+(?!-)/$1/g;
+
+ # Ignore anything inside single quotes; it could be an
+ # argument to grep or the like.
+ $line =~ s/(^|[^\\\"](?:\\\\)*)\'(?:\\.|[^\\\'])+\'/$1''/g;
+
+ # As above, with the exception that we don't remove the string
+ # if the quote is immediately preceded by a < or a -, so we
+ # can match "foo <<-?'xyz'" as a heredoc later
+ # The check is a little more greedy than we'd like, but the
+ # heredoc test itself will weed out any false positives
+ $cat_line =~ s/(^|[^<\\\"-](?:\\\\)*)\'(?:\\.|[^\\\'])+\'/$1''/g;
+
+ $re = '(?<![\$\\\])\$\"[^\"]+\"';
+ if ($line =~ m/(.*)($re)/o) {
+ my $count = () = $1 =~ /(^|[^\\])\"/g;
+ if ($count % 2 == 0) {
+ output_explanation($display_filename, $orig_line,
+ q<$"foo" should be eval_gettext "foo">);
+ }
+ }
+
+ foreach my $re (@string_bashisms_keys) {
+ my $expl = $string_bashisms{$re};
+ if ($line =~ m/($re)/) {
+ $found = 1;
+ $match = $1;
+ $explanation = $expl;
+ output_explanation($display_filename, $orig_line,
+ $explanation);
+ }
+ }
+
+ # We've checked for all the things we still want to notice in
+ # double-quoted strings, so now remove those strings as well.
+ $line =~ s/(^|[^\\\'](?:\\\\)*)\"(?:\\.|[^\\\"])+\"/$1""/g;
+ $cat_line =~ s/(^|[^<\\\'-](?:\\\\)*)\"(?:\\.|[^\\\"])+\"/$1""/g;
+ foreach my $re (@bashisms_keys) {
+ my $expl = $bashisms{$re};
+ if ($line =~ m/($re)/) {
+ $found = 1;
+ $match = $1;
+ $explanation = $expl;
+ output_explanation($display_filename, $orig_line,
+ $explanation);
+ }
+ }
+ # This check requires the value to be compared, which could
+ # be done in the regex itself but requires "use re 'eval'".
+ # So it's better done in its own
+ if ($line =~ m/$LEADIN((?:exit|return)\s+(\d{3,}))/o && $2 > 255) {
+ $explanation = 'exit|return status code greater than 255';
+ output_explanation($display_filename, $orig_line,
+ $explanation);
+ }
+
+ # Only look for the beginning of a heredoc here, after we've
+ # stripped out quoted material, to avoid false positives.
+ if ($cat_line
+ =~ m/(?:^|[^<])\<\<(\-?)\s*(?:(?!<|'|")((?:[^\s;>|]+(?:(?<=\\)[\s;>|])?)+)|[\'\"](.*?)[\'\"])/
+ ) {
+ $cat_indented = ($1 && $1 eq '-') ? 1 : 0;
+ my $quoted = defined($3);
+ $cat_string = $quoted ? $3 : $2;
+ unless ($quoted) {
+ # Now strip backslashes. Keep the position of the
+ # last match in a variable, as s/// resets it back
+ # to undef, but we don't want that.
+ my $pos = 0;
+ pos($cat_string) = $pos;
+ while ($cat_string =~ s/\G(.*?)\\/$1/) {
+ # position += length of match + the character
+ # that followed the backslash:
+ $pos += length($1) + 1;
+ pos($cat_string) = $pos;
+ }
+ }
+ $start_lines{'cat_string'} = $.;
+ }
+ }
+ }
+
+ warn
+"error: $display_filename: Unterminated heredoc found, EOF reached. Wanted: <$cat_string>, opened in line $start_lines{'cat_string'}\n"
+ if ($cat_string ne '');
+ warn
+"error: $display_filename: Unterminated quoted string found, EOF reached. Wanted: <$quote_string>, opened in line $start_lines{'quote_string'}\n"
+ if ($quote_string ne '');
+ warn "error: $display_filename: EOF reached while on line continuation.\n"
+ if ($buffered_line ne '');
+
+ close C;
+
+ if ($mode && !$issues) {
+ warn "could not find any possible bashisms in bash script $filename\n";
+ $status |= 4;
+ }
+}
+
+exit $status;
+
+sub output_explanation {
+ my ($filename, $line, $explanation) = @_;
+
+ if ($mode) {
+ # When examining a bash script, just flag that there are indeed
+ # bashisms present
+ $issues = 1;
+ } else {
+ warn "possible bashism in $filename line $. ($explanation):\n$line\n";
+ if ($opt_early_fail) {
+ exit 1;
+ }
+ $status |= 1;
+ }
+}
+
+# Returns non-zero if the given file is not actually a shell script,
+# just looks like one.
+sub script_is_evil_and_wrong {
+ my ($filename) = @_;
+ my $ret = -1;
+ # lintian's version of this function aborts if the file
+ # can't be opened, but we simply return as the next
+ # test in the calling code handles reporting the error
+ # itself
+ open(IN, '<', $filename) or return $ret;
+ my $i = 0;
+ my $var = "0";
+ my $backgrounded = 0;
+ local $_;
+ while (<IN>) {
+ chomp;
+ next if /^#/o;
+ next if /^$/o;
+ last if (++$i > 55);
+ if (
+ m~
+ # the exec should either be "eval"ed or a new statement
+ (^\s*|\beval\s*[\'\"]|(;|&&|\b(then|else))\s*)
+
+ # eat anything between the exec and $0
+ exec\s*.+\s*
+
+ # optionally quoted executable name (via $0)
+ .?\$$var.?\s*
+
+ # optional "end of options" indicator
+ (--\s*)?
+
+ # Match expressions of the form '${1+$@}', '${1:+"$@"',
+ # '"${1+$@', "$@", etc where the quotes (before the dollar
+ # sign(s)) are optional and the second (or only if the $1
+ # clause is omitted) parameter may be $@ or $*.
+ #
+ # Finally the whole subexpression may be omitted for scripts
+ # which do not pass on their parameters (i.e. after re-execing
+ # they take their parameters (and potentially data) from stdin
+ .?(\$\{1:?\+.?)?(\$(\@|\*))?~x
+ ) {
+ $ret = $. - 1;
+ last;
+ } elsif (/^\s*(\w+)=\$0;/) {
+ $var = $1;
+ } elsif (
+ m~
+ # Match scripts which use "foo $0 $@ &\nexec true\n"
+ # Program name
+ \S+\s+
+
+ # As above
+ .?\$$var.?\s*
+ (--\s*)?
+ .?(\$\{1:?\+.?)?(\$(\@|\*))?.?\s*\&~x
+ ) {
+
+ $backgrounded = 1;
+ } elsif (
+ $backgrounded
+ and m~
+ # the exec should either be "eval"ed or a new statement
+ (^\s*|\beval\s*[\'\"]|(;|&&|\b(then|else))\s*)
+ exec\s+true(\s|\Z)~x
+ ) {
+
+ $ret = $. - 1;
+ last;
+ } elsif (m~\@DPATCH\@~) {
+ $ret = $. - 1;
+ last;
+ }
+
+ }
+ close IN;
+ return $ret;
+}
+
+sub init_hashes {
+
+ %bashisms = (
+ qr'(?:^|\s+)function [^<>\(\)\[\]\{\};|\s]+(\s|\(|\Z)' =>
+ q<'function' is useless>,
+ $LEADIN . qr'select\s+\w+' => q<'select' is not POSIX>,
+ qr'(test|-o|-a)\s*[^\s]+\s+==\s' => q<should be 'b = a'>,
+ qr'\[\s+[^\]]+\s+==\s' => q<should be 'b = a'>,
+ qr'\s\|\&' => q<pipelining is not POSIX>,
+ qr'[^\\\$]\{([^\s\\\}]*?,)+[^\\\}\s]*\}' => q<brace expansion>,
+ qr'\{\d+\.\.\d+(?:\.\.\d+)?\}' =>
+ q<brace expansion, {a..b[..c]}should be $(seq a [c] b)>,
+ qr'(?i)\{[a-z]\.\.[a-z](?:\.\.\d+)?\}' => q<brace expansion>,
+ qr'(?:^|\s+)\w+\[\d+\]=' => q<bash arrays, H[0]>,
+ $LEADIN
+ . qr'read\s+(?:-[a-qs-zA-Z\d-]+)' =>
+ q<read with option other than -r>,
+ $LEADIN
+ . qr'read\s*(?:-\w+\s*)*(?:\".*?\"|[\'].*?[\'])?\s*(?:;|$)' =>
+ q<read without variable>,
+ $LEADIN . qr'echo\s+(-n\s+)?-n?en?\s' => q<echo -e>,
+ $LEADIN . qr'exec\s+-[acl]' => q<exec -c/-l/-a name>,
+ $LEADIN . qr'let\s' => q<let ...>,
+ qr'(?<![\$\(])\(\(.*\)\)' => q<'((' should be '$(('>,
+ qr'(?:^|\s+)(\[|test)\s+-a' => q<test with unary -a (should be -e)>,
+ qr'\&>' => q<should be \>word 2\>&1>,
+ qr'(<\&|>\&)\s*((-|\d+)[^\s;|)}`&\\\\]|[^-\d\s]+(?<!\$)(?!\d))' =>
+ q<should be \>word 2\>&1>,
+ qr'\[\[(?!:)' =>
+ q<alternative test command ([[ foo ]] should be [ foo ])>,
+ qr'/dev/(tcp|udp)' => q</dev/(tcp|udp)>,
+ $LEADIN . qr'builtin\s' => q<builtin>,
+ $LEADIN . qr'caller\s' => q<caller>,
+ $LEADIN . qr'compgen\s' => q<compgen>,
+ $LEADIN . qr'complete\s' => q<complete>,
+ $LEADIN . qr'declare\s' => q<declare>,
+ $LEADIN . qr'dirs(\s|\Z)' => q<dirs>,
+ $LEADIN . qr'disown\s' => q<disown>,
+ $LEADIN . qr'enable\s' => q<enable>,
+ $LEADIN . qr'mapfile\s' => q<mapfile>,
+ $LEADIN . qr'readarray\s' => q<readarray>,
+ $LEADIN . qr'shopt(\s|\Z)' => q<shopt>,
+ $LEADIN . qr'suspend\s' => q<suspend>,
+ $LEADIN . qr'time\s' => q<time>,
+ $LEADIN . qr'type\s' => q<type>,
+ $LEADIN . qr'typeset\s' => q<typeset>,
+ $LEADIN . qr'ulimit(\s|\Z)' => q<ulimit>,
+ $LEADIN . qr'set\s+-[BHT]+' => q<set -[BHT]>,
+ $LEADIN . qr'alias\s+-p' => q<alias -p>,
+ $LEADIN . qr'unalias\s+-a' => q<unalias -a>,
+ $LEADIN . qr'local\s+-[a-zA-Z]+' => q<local -opt>,
+ # function '=' is special-cased due to bash arrays (think of "foo=()")
+ qr'(?:^|\s)\s*=\s*\(\s*\)\s*([\{|\(]|\Z)' =>
+ q<function names should only contain [a-z0-9_]>,
+qr'(?:^|\s)(?<func>function\s)?\s*(?:[^<>\(\)\[\]\{\};|\s]*[^<>\(\)\[\]\{\};|\s\w][^<>\(\)\[\]\{\};|\s]*)(?(<func>)(?=)|(?<!=))\s*(?(<func>)(?:\(\s*\))?|\(\s*\))\s*([\{|\(]|\Z)'
+ => q<function names should only contain [a-z0-9_]>,
+ $LEADIN . qr'(push|pop)d(\s|\Z)' => q<(push|pop)d>,
+ $LEADIN . qr'export\s+-[^p]' => q<export only takes -p as an option>,
+ qr'(?:^|\s+)[<>]\(.*?\)' => q<\<() process substitution>,
+ $LEADIN . qr'readonly\s+-[af]' => q<readonly -[af]>,
+ $LEADIN . qr'(sh|\$\{?SHELL\}?) -[rD]' => q<sh -[rD]>,
+ $LEADIN . qr'(sh|\$\{?SHELL\}?) --\w+' => q<sh --long-option>,
+ $LEADIN . qr'(sh|\$\{?SHELL\}?) [-+]O' => q<sh [-+]O>,
+ qr'\[\^[^]]+\]' => q<[^] should be [!]>,
+ $LEADIN
+ . qr'printf\s+-v' =>
+ q<'printf -v var ...' should be var='$(printf ...)'>,
+ $LEADIN . qr'coproc\s' => q<coproc>,
+ qr';;?&' => q<;;& and ;& special case operators>,
+ $LEADIN . qr'jobs\s' => q<jobs>,
+ # $LEADIN . qr'jobs\s+-[^lp]\s' => q<'jobs' with option other than -l or -p>,
+ $LEADIN
+ . qr'command\s+(?:-[pvV]+\s+)*-(?:[pvV])*[^pvV\s]' =>
+ q<'command' with option other than -p, -v or -V>,
+ $LEADIN
+ . qr'setvar\s' =>
+ q<setvar 'foo' 'bar' should be eval 'foo="'"$bar"'"'>,
+ $LEADIN
+ . qr'trap\s+["\']?.*["\']?\s+.*(?:ERR|DEBUG|RETURN)' =>
+ q<trap with ERR|DEBUG|RETURN>,
+ $LEADIN
+ . qr'(?:exit|return)\s+-\d' =>
+ q<exit|return with negative status code>,
+ $LEADIN
+ . qr'(?:exit|return)\s+--' =>
+ q<'exit --' should be 'exit' (idem for return)>,
+ $LEADIN . qr'hash(\s|\Z)' => q<hash>,
+ qr'(?:[:=\s])~(?:[+-]|[+-]?\d+)(?:[/\s]|\Z)' =>
+ q<non-standard tilde expansion>,
+ );
+
+ %string_bashisms = (
+ qr'\$\[[^][]+\]' => q<'$[' should be '$(('>,
+ qr'\$\{(?:\w+|@|\*)\:(?:\d+|\$\{?\w+\}?)+(?::(?:\d+|\$\{?\w+\}?)+)?\}'
+ => q<${foo:3[:1]}>,
+ qr'\$\{!\w+[\@*]\}' => q<${!prefix[*|@]>,
+ qr'\$\{!\w+\}' => q<${!name}>,
+ qr'\$\{(?:\w+|@|\*)([,^]{1,2}.*?)\}' =>
+ q<${parm,[,][pat]} or ${parm^[^][pat]}>,
+ qr'\$\{[@*]([#%]{1,2}.*?)\}' => q<${[@|*]#[#]pat} or ${[@|*]%[%]pat}>,
+ qr'\$\{#[@*]\}' => q<${#@} or ${#*}>,
+ qr'\$\{(?:\w+|@|\*)(/.+?){1,2}\}' => q<${parm/?/pat[/str]}>,
+ qr'\$\{\#?\w+\[.+\](?:[/,:#%^].+?)?\}' =>
+ q<bash arrays, ${name[0|*|@]}>,
+ qr'\$\{?RANDOM\}?\b' => q<$RANDOM>,
+ qr'\$\{?(OS|MACH)TYPE\}?\b' => q<$(OS|MACH)TYPE>,
+ qr'\$\{?HOST(TYPE|NAME)\}?\b' => q<$HOST(TYPE|NAME)>,
+ qr'\$\{?DIRSTACK\}?\b' => q<$DIRSTACK>,
+ qr'\$\{?EUID\}?\b' => q<$EUID should be "$(id -u)">,
+ qr'\$\{?UID\}?\b' => q<$UID should be "$(id -ru)">,
+ qr'\$\{?SECONDS\}?\b' => q<$SECONDS>,
+ qr'\$\{?BASH_[A-Z]+\}?\b' => q<$BASH_SOMETHING>,
+ qr'\$\{?SHELLOPTS\}?\b' => q<$SHELLOPTS>,
+ qr'\$\{?PIPESTATUS\}?\b' => q<$PIPESTATUS>,
+ qr'\$\{?SHLVL\}?\b' => q<$SHLVL>,
+ qr'\$\{?FUNCNAME\}?\b' => q<$FUNCNAME>,
+ qr'\$\{?TMOUT\}?\b' => q<$TMOUT>,
+ qr'(?:^|\s+)TMOUT=' => q<TMOUT=>,
+ qr'\$\{?TIMEFORMAT\}?\b' => q<$TIMEFORMAT>,
+ qr'(?:^|\s+)TIMEFORMAT=' => q<TIMEFORMAT=>,
+ qr'(?<![$\\])\$\{?_\}?\b' => q<$_>,
+ qr'(?:^|\s+)GLOBIGNORE=' => q<GLOBIGNORE=>,
+ qr'<<<' => q<\<\<\< here string>,
+ $LEADIN
+ . qr'echo\s+(?:-[^e\s]+\s+)?\"[^\"]*(\\[abcEfnrtv0])+.*?[\"]' =>
+ q<unsafe echo with backslash>,
+ qr'\$\(\([\s\w$*/+-]*\w\+\+.*?\)\)' =>
+ q<'$((n++))' should be '$n; $((n=n+1))'>,
+ qr'\$\(\([\s\w$*/+-]*\+\+\w.*?\)\)' =>
+ q<'$((++n))' should be '$((n=n+1))'>,
+ qr'\$\(\([\s\w$*/+-]*\w\-\-.*?\)\)' =>
+ q<'$((n--))' should be '$n; $((n=n-1))'>,
+ qr'\$\(\([\s\w$*/+-]*\-\-\w.*?\)\)' =>
+ q<'$((--n))' should be '$((n=n-1))'>,
+ qr'\$\(\([\s\w$*/+-]*\*\*.*?\)\)' => q<exponentiation is not POSIX>,
+ $LEADIN . qr'printf\s["\'][^"\']*?%q.+?["\']' => q<printf %q>,
+ );
+
+ %singlequote_bashisms = (
+ $LEADIN
+ . qr'echo\s+(?:-[^e\s]+\s+)?\'[^\']*(\\[abcEfnrtv0])+.*?[\']' =>
+ q<unsafe echo with backslash>,
+ $LEADIN
+ . qr'source\s+[\"\']?(?:\.\/|\/|\$|[\w~.-])\S*' =>
+ q<should be '.', not 'source'>,
+ );
+
+ if ($opt_echo) {
+ $bashisms{ $LEADIN . qr'echo\s+-[A-Za-z]*n' } = q<echo -n>;
+ }
+ if ($opt_posix) {
+ $bashisms{ $LEADIN . qr'local\s+\w+(\s+\W|\s*[;&|)]|$)' }
+ = q<local foo>;
+ $bashisms{ $LEADIN . qr'local\s+\w+=' } = q<local foo=bar>;
+ $bashisms{ $LEADIN . qr'local\s+\w+\s+\w+' } = q<local x y>;
+ $bashisms{ $LEADIN . qr'((?:test|\[)\s+.+\s-[ao])\s' } = q<test -a/-o>;
+ $bashisms{ $LEADIN . qr'kill\s+-[^sl]\w*' } = q<kill -[0-9] or -[A-Z]>;
+ $bashisms{ $LEADIN . qr'trap\s+["\']?.*["\']?\s+.*[1-9]' }
+ = q<trap with signal numbers>;
+ }
+
+ if ($makefile) {
+ $string_bashisms{qr'(\$\(|\`)\s*\<\s*([^\s\)]{2,}|[^DF])\s*(\)|\`)'}
+ = q<'$(\< foo)' should be '$(cat foo)'>;
+ } else {
+ $bashisms{ $LEADIN . qr'\w+\+=' } = q<should be VAR="${VAR}foo">;
+ $string_bashisms{qr'(\$\(|\`)\s*\<\s*\S+\s*(\)|\`)'}
+ = q<'$(\< foo)' should be '$(cat foo)'>;
+ }
+
+ if ($opt_extra) {
+ $string_bashisms{qr'\$\{?BASH\}?\b'} = q<$BASH>;
+ $string_bashisms{qr'(?:^|\s+)RANDOM='} = q<RANDOM=>;
+ $string_bashisms{qr'(?:^|\s+)(OS|MACH)TYPE='} = q<(OS|MACH)TYPE=>;
+ $string_bashisms{qr'(?:^|\s+)HOST(TYPE|NAME)='} = q<HOST(TYPE|NAME)=>;
+ $string_bashisms{qr'(?:^|\s+)DIRSTACK='} = q<DIRSTACK=>;
+ $string_bashisms{qr'(?:^|\s+)EUID='} = q<EUID=>;
+ $string_bashisms{qr'(?:^|\s+)UID='} = q<UID=>;
+ $string_bashisms{qr'(?:^|\s+)BASH(_[A-Z]+)?='} = q<BASH(_SOMETHING)=>;
+ $string_bashisms{qr'(?:^|\s+)SHELLOPTS='} = q<SHELLOPTS=>;
+ $string_bashisms{qr'\$\{?POSIXLY_CORRECT\}?\b'} = q<$POSIXLY_CORRECT>;
+ }
+}
--
2.33.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [LTP] [PATCH 3/4] rules.mk: Add checkbashisms to 'make check' for *.sh
2021-09-02 10:37 [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
2021-09-02 10:37 ` [LTP] [PATCH 1/4] doc: Mention make check Petr Vorel
2021-09-02 10:37 ` [LTP] [PATCH 2/4] Vendor checkbashisms.pl version 2.20.5 Petr Vorel
@ 2021-09-02 10:37 ` Petr Vorel
2021-09-02 13:27 ` Cyril Hrubis
2021-09-02 10:37 ` [LTP] [PATCH 4/4] doc: Update for vendored checkbashisms.pl Petr Vorel
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 10:37 UTC (permalink / raw)
To: ltp
Similarly to 3c83485d1 it runs checkbashisms part of 'make check' and
'make check-$TCID.sh' for particular script only, e.g. 'make
check-tst_net.sh' (deliberately kept *.sh suffix to be different for C
targets, because shell targets itself does not have make target).
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
include/mk/env_post.mk | 2 ++
include/mk/generic_leaf_target.inc | 2 +-
include/mk/generic_trunk_target.inc | 2 +-
include/mk/rules.mk | 9 +++++++++
4 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/mk/env_post.mk b/include/mk/env_post.mk
index eb76b38a4..ec045c40d 100644
--- a/include/mk/env_post.mk
+++ b/include/mk/env_post.mk
@@ -93,6 +93,8 @@ CHECK_TARGETS ?= $(addprefix check-,$(notdir $(patsubst %.c,%,$(sort $(wildcar
CHECK_TARGETS := $(filter-out $(addprefix check-, $(FILTER_OUT_MAKE_TARGETS)), $(CHECK_TARGETS))
CHECK ?= $(abs_top_srcdir)/tools/sparse/sparse-ltp
CHECK_NOFLAGS ?= $(abs_top_srcdir)/scripts/checkpatch.pl -f --no-tree --terse --no-summary --ignore CONST_STRUCT,VOLATILE,SPLIT_STRING
+SHELL_CHECK ?= $(abs_top_srcdir)/scripts/checkbashisms.pl --force --extra
+SHELL_CHECK_TARGETS ?= $(addprefix check-,$(notdir $(sort $(wildcard $(abs_srcdir)/*.sh))))
ifeq ($(CHECK),$(abs_top_srcdir)/tools/sparse/sparse-ltp)
CHECK_DEPS += $(CHECK)
diff --git a/include/mk/generic_leaf_target.inc b/include/mk/generic_leaf_target.inc
index aa092a5a3..33e9c9ea0 100644
--- a/include/mk/generic_leaf_target.inc
+++ b/include/mk/generic_leaf_target.inc
@@ -110,6 +110,6 @@ $(INSTALL_FILES): | $(INSTALL_DEPS)
install: $(INSTALL_FILES)
$(CHECK_TARGETS): | $(CHECK_DEPS)
-check: $(CHECK_TARGETS)
+check: $(CHECK_TARGETS) $(SHELL_CHECK_TARGETS)
# vim: syntax=make
diff --git a/include/mk/generic_trunk_target.inc b/include/mk/generic_trunk_target.inc
index 32a108fbf..82aece7c0 100644
--- a/include/mk/generic_trunk_target.inc
+++ b/include/mk/generic_trunk_target.inc
@@ -69,7 +69,7 @@ $(INSTALL_FILES): | $(INSTALL_DEPS)
trunk-install: $(INSTALL_FILES)
$(CHECK_TARGETS): | $(CHECK_DEPS)
-trunk-check: $(CHECK_TARGETS)
+trunk-check: $(CHECK_TARGETS) $(SHELL_CHECK_TARGETS)
# Avoid creating duplicate .PHONY references to all, clean, and install. IIRC,
# I've seen some indeterministic behavior when one does this in the past with
diff --git a/include/mk/rules.mk b/include/mk/rules.mk
index 6bd184841..a60e6705a 100644
--- a/include/mk/rules.mk
+++ b/include/mk/rules.mk
@@ -48,3 +48,12 @@ else
@-$(CHECK_NOFLAGS) $<
@-$(CHECK) $(CHECK_FLAGS) $(CPPFLAGS) $(CFLAGS) $<
endif
+
+.PHONY: $(SHELL_CHECK_TARGETS)
+$(SHELL_CHECK_TARGETS): check-%.sh: %.sh
+ifdef VERBOSE
+ -$(SHELL_CHECK) $<
+else
+ @echo CHECK $(target_rel_dir)$<
+ @-$(SHELL_CHECK) $<
+endif
--
2.33.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [LTP] [PATCH 4/4] doc: Update for vendored checkbashisms.pl
2021-09-02 10:37 [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
` (2 preceding siblings ...)
2021-09-02 10:37 ` [LTP] [PATCH 3/4] rules.mk: Add checkbashisms to 'make check' for *.sh Petr Vorel
@ 2021-09-02 10:37 ` Petr Vorel
2021-09-02 13:29 ` Cyril Hrubis
2021-09-02 11:50 ` [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
2021-09-02 14:01 ` Joerg Vehlow
5 siblings, 1 reply; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 10:37 UTC (permalink / raw)
To: ltp
in previous commit.
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
doc/maintainer-patch-review-checklist.txt | 3 +--
doc/test-writing-guidelines.txt | 8 ++++++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/doc/maintainer-patch-review-checklist.txt b/doc/maintainer-patch-review-checklist.txt
index c8ace80f7..c7bb47810 100644
--- a/doc/maintainer-patch-review-checklist.txt
+++ b/doc/maintainer-patch-review-checklist.txt
@@ -49,8 +49,7 @@ New test should
### Shell tests
* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell[shell API]
-* Check coding style with
- https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl[checkbashism.pl]
+* Check coding style with `make check`
(more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style[Shell coding style])
* If a test is a regression test it should include related kernel or glibc commits as a comment
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 8053f0cb0..b87446d1b 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -150,9 +150,13 @@ to 'dash' by default or install 'dash' on your favorite distribution and use
it to run the tests. If your distribution lacks 'dash' package you can always
compile it from http://gondor.apana.org.au/~herbert/dash/files/[source].
-Debian also has nice devscript
+Run `make check` in the test's directory and/or use `make check-$TCID.sh`,
+it uses (among other checks) our vendored version of
https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl[checkbashism.pl]
-that can be used to check for non-portable shell code.
+from Debian, that is used to check for non-portable shell code.
+
+NOTE: If `make check` does not report any problems, the code still may be wrong
+ as `checkbashisms.pl` used for checking only looks for common mistakes.
Here are some common sense style rules for shell
--
2.33.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
2021-09-02 10:37 [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
` (3 preceding siblings ...)
2021-09-02 10:37 ` [LTP] [PATCH 4/4] doc: Update for vendored checkbashisms.pl Petr Vorel
@ 2021-09-02 11:50 ` Petr Vorel
2021-09-02 14:01 ` Joerg Vehlow
5 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 11:50 UTC (permalink / raw)
To: ltp
Hi All,
> checkbashisms.pl has problem with type. Although it's in POSIX [1] even
> in old one from 2004 [2] and it's supported by all common shells (i.e.
> bash, zsh, dash, busybox sh, mksh; even in ksh; maybe just csh does not
> support it) checkbashisms.pl complains about it:
> $ make check-tst_test.sh
> CHECK testcases/lib/tst_test.sh
> possible bashism in tst_test.sh line 33 (type):
> if type $TST_CLEANUP >/dev/null 2>/dev/null; then
> possible bashism in tst_test.sh line 694 (type):
> if type $TST_SETUP >/dev/null 2>/dev/null; then
> possible bashism in tst_test.sh line 726 (type):
> if type ${TST_TESTFUNC}1 > /dev/null 2>&1; then
> make: [../../include/mk/rules.mk:58: check-tst_test.sh] Error 1 (ignored)
> Should I report it to Debian (the upstream)? Or at least ask for way to
> suppress the warning?
type is part of POSIX, but as part of the X/Open Systems Interfaces option
(XSI). The checkbashisms man page explicitly says:
Note that the definition of a bashism in this context roughly equates to "a
shell feature that is not required to be supported by POSIX"; this means that
some issues flagged may be permitted under optional sections of POSIX, such as
XSI or User Portability.
=> type is flagged because it is an optional feature.
I just send a patch which disabled it from source code.
Kind regards,
Petr
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 1/4] doc: Mention make check
2021-09-02 10:37 ` [LTP] [PATCH 1/4] doc: Mention make check Petr Vorel
@ 2021-09-02 13:17 ` Cyril Hrubis
2021-09-02 15:46 ` Petr Vorel
0 siblings, 1 reply; 20+ messages in thread
From: Cyril Hrubis @ 2021-09-02 13:17 UTC (permalink / raw)
To: ltp
Hi!
> --------------------------------------------------------------------------------
> statvfs01 statvfs01
> statvfs02 statvfs02
> -
> +
> +statx01 statx01
> +
> stime01 stime01
> stime02 stime02
> -
> +
If we were pedantic this should probably be in a separate patch...
Otherwise the changes looks good, thanks for fixing this.
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 2/4] Vendor checkbashisms.pl version 2.20.5
2021-09-02 10:37 ` [LTP] [PATCH 2/4] Vendor checkbashisms.pl version 2.20.5 Petr Vorel
@ 2021-09-02 13:25 ` Cyril Hrubis
0 siblings, 0 replies; 20+ messages in thread
From: Cyril Hrubis @ 2021-09-02 13:25 UTC (permalink / raw)
To: ltp
Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 3/4] rules.mk: Add checkbashisms to 'make check' for *.sh
2021-09-02 10:37 ` [LTP] [PATCH 3/4] rules.mk: Add checkbashisms to 'make check' for *.sh Petr Vorel
@ 2021-09-02 13:27 ` Cyril Hrubis
2021-09-02 15:51 ` Petr Vorel
0 siblings, 1 reply; 20+ messages in thread
From: Cyril Hrubis @ 2021-09-02 13:27 UTC (permalink / raw)
To: ltp
Hi!
Looks good.
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 4/4] doc: Update for vendored checkbashisms.pl
2021-09-02 10:37 ` [LTP] [PATCH 4/4] doc: Update for vendored checkbashisms.pl Petr Vorel
@ 2021-09-02 13:29 ` Cyril Hrubis
2021-09-09 10:55 ` Petr Vorel
0 siblings, 1 reply; 20+ messages in thread
From: Cyril Hrubis @ 2021-09-02 13:29 UTC (permalink / raw)
To: ltp
Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
2021-09-02 10:37 [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
` (4 preceding siblings ...)
2021-09-02 11:50 ` [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
@ 2021-09-02 14:01 ` Joerg Vehlow
2021-09-02 15:09 ` Petr Vorel
2021-09-02 15:14 ` Petr Vorel
5 siblings, 2 replies; 20+ messages in thread
From: Joerg Vehlow @ 2021-09-02 14:01 UTC (permalink / raw)
To: ltp
Hi
one general question about this: How to we want to handle false-positives?
e.g.:
$ checkbashisms testcases/kernel/controllers/memcg/functional/memcg_lib.sh
possible bashism in
testcases/kernel/controllers/memcg/functional/memcg_lib.sh line 387
('((' should be '$(('):
??????? local limit_down=$(( PAGESIZE * ((limit + PAGESIZE - 1) /
PAGESIZE) ))
This is obviously a false positive, but could probably be adding a space
between the brackets.
or
$ checkbashisms testcases/kernel/connectors/pec/cn_pec.sh
possible bashism in testcases/kernel/connectors/pec/cn_pec.sh line 127
(should be >word 2>&1):
??????????????? done <&${fd_act}
This one is just a false positive and I have no clue how to prevent this.
I think the script does not like the <&, but this is posix...
Joerg
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
2021-09-02 14:01 ` Joerg Vehlow
@ 2021-09-02 15:09 ` Petr Vorel
2021-09-03 4:28 ` Joerg Vehlow
2021-09-02 15:14 ` Petr Vorel
1 sibling, 1 reply; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 15:09 UTC (permalink / raw)
To: ltp
Hi Joerg,
> Hi
> one general question about this: How to we want to handle false-positives?
Good point, thanks! Generally we can disable things which does not work for us.
I'd be pragmatic, if something works on most of shells and let's disable it,
just not disable needed test just due one false positive.
> e.g.:
> $ checkbashisms testcases/kernel/controllers/memcg/functional/memcg_lib.sh
> possible bashism in
> testcases/kernel/controllers/memcg/functional/memcg_lib.sh line 387 ('(('
> should be '$(('):
> ??????? local limit_down=$(( PAGESIZE * ((limit + PAGESIZE - 1) / PAGESIZE)
> ))
> This is obviously a false positive, but could probably be adding a space
> between the brackets.
The only thing how to get away this was to introduce another variable:
local limit_psize=$((limit + PAGESIZE - 1))
local limit_down=$((PAGESIZE * (limit_psize / PAGESIZE)))
I'm not sure if it's not POSIX, but works because supported by all shells
(similar case to 'typo' not being POSIX but POSIX extensions). Maybe we should
report it.
> or
> $ checkbashisms testcases/kernel/connectors/pec/cn_pec.sh
> possible bashism in testcases/kernel/connectors/pec/cn_pec.sh line 127
> (should be >word 2>&1):
> ??????????????? done <&${fd_act}
> This one is just a false positive and I have no clue how to prevent this.
> I think the script does not like the <&, but this is posix...
The same here, I'm not sure if it's POSIX. &> definitely is not POSIX.
I remember we were talking about it. Can we avoid it somehow?
Kind regards,
Petr
> Joerg
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
2021-09-02 14:01 ` Joerg Vehlow
2021-09-02 15:09 ` Petr Vorel
@ 2021-09-02 15:14 ` Petr Vorel
1 sibling, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 15:14 UTC (permalink / raw)
To: ltp
Hi,
The only thing about checkbashisms.pl I don't like is that upstream is hosted in
Debian. reportbug is ok, but sometimes it takes time to get responses to Debian
bugs. script is part of devscripts, where many scripts are. And there is no way
to report via https://salsa.debian.org/. Thus talking to upstream can takes
time.
Kind regards,
Petr
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 1/4] doc: Mention make check
2021-09-02 13:17 ` Cyril Hrubis
@ 2021-09-02 15:46 ` Petr Vorel
0 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 15:46 UTC (permalink / raw)
To: ltp
Hi Cyril,
> Hi!
> > --------------------------------------------------------------------------------
> > statvfs01 statvfs01
> > statvfs02 statvfs02
> > -
> > +
> > +statx01 statx01
> > +
> > stime01 stime01
> > stime02 stime02
> > -
> > +
> If we were pedantic this should probably be in a separate patch...
Yep. I removed this change (unintentional, my editor setup) and merged.
Thanks for review!
Kind regards,
Petr
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 3/4] rules.mk: Add checkbashisms to 'make check' for *.sh
2021-09-02 13:27 ` Cyril Hrubis
@ 2021-09-02 15:51 ` Petr Vorel
0 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2021-09-02 15:51 UTC (permalink / raw)
To: ltp
> Hi!
> Looks good.
Thx!
I wonder if we should use --early-fail, not sure what was the benefit of being
added.
Kind regards,
Petr
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
2021-09-02 15:09 ` Petr Vorel
@ 2021-09-03 4:28 ` Joerg Vehlow
2021-09-03 7:43 ` Petr Vorel
0 siblings, 1 reply; 20+ messages in thread
From: Joerg Vehlow @ 2021-09-03 4:28 UTC (permalink / raw)
To: ltp
H Petr,
On 9/2/2021 5:09 PM, Petr Vorel wrote:
>
>> $ checkbashisms testcases/kernel/connectors/pec/cn_pec.sh
>> possible bashism in testcases/kernel/connectors/pec/cn_pec.sh line 127
>> (should be >word 2>&1):
>> ??????????????? done <&${fd_act}
>> This one is just a false positive and I have no clue how to prevent this.
>> I think the script does not like the <&, but this is posix...
> The same here, I'm not sure if it's POSIX. &> definitely is not POSIX.
> I remember we were talking about it. Can we avoid it somehow?
<&n is the only way to use the file handle n for input. <n would use the
file n.
See:
https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html#tag_02_07_05
checkbashisms has no problems if n is a number, not a variable. There is
nothing in the standard about n being a variable, but I guess this
should be posix as well.
Furthermore the suggested fix "(should be >word 2>&1)" clearly shows,
that checkbashisms thinks, this is &>.
I don't see another way to implement this (but using an implementation
in c). And I am not really happy to bend my code around bugs in a tool,
that is supposed to ensure better code.
I'd rather try to fix checkbashims in this case. Even the ((-case should
be fixed, after checking if it is posix. The suggestion (replace with
"$((") indicates at least a bug in the tool.
To be honest, I am a bit disappointed from this tool. It doesn't seem to
be tested very well... Probably barely good enough for scripting in the
kernel.
Joerg
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
2021-09-03 4:28 ` Joerg Vehlow
@ 2021-09-03 7:43 ` Petr Vorel
2021-09-03 8:10 ` Joerg Vehlow
0 siblings, 1 reply; 20+ messages in thread
From: Petr Vorel @ 2021-09-03 7:43 UTC (permalink / raw)
To: ltp
Hi Joerg,
> H Petr,
> On 9/2/2021 5:09 PM, Petr Vorel wrote:
> > > $ checkbashisms testcases/kernel/connectors/pec/cn_pec.sh
> > > possible bashism in testcases/kernel/connectors/pec/cn_pec.sh line 127
> > > (should be >word 2>&1):
> > > ??????????????? done <&${fd_act}
> > > This one is just a false positive and I have no clue how to prevent this.
> > > I think the script does not like the <&, but this is posix...
> > The same here, I'm not sure if it's POSIX. &> definitely is not POSIX.
> > I remember we were talking about it. Can we avoid it somehow?
> <&n is the only way to use the file handle n for input. <n would use the
> file n.
> See: https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html#tag_02_07_05
> checkbashisms has no problems if n is a number, not a variable. There is
> nothing in the standard about n being a variable, but I guess this should be
> posix as well.
> Furthermore the suggested fix "(should be >word 2>&1)" clearly shows, that
> checkbashisms thinks, this is &>.
Agree, it's very likely checkbashims bug which should be fixed.
> I don't see another way to implement this (but using an implementation in
> c). And I am not really happy to bend my code around bugs in a tool, that is
> supposed to ensure better code.
+1
> I'd rather try to fix checkbashims in this case. Even the ((-case should be
> fixed, after checking if it is posix. The suggestion (replace with "$((")
> indicates at least a bug in the tool.
I'll try to search for $(( )) in POSIX docs.
What is the outcome? Should I first fix checkbashisms before merging these?
I suggest to merge it for local checking (similar to checkpatch.pl for C).
Because it cannot be used in CI right now, not even because these 2 false
positives, but because not everything has been converted to use new shell API.
> To be honest, I am a bit disappointed from this tool. It doesn't seem to be
> tested very well... Probably barely good enough for scripting in the kernel.
This tool comes from Debian, written long time ago (2009) for release goal to
use dash as /bin/sh [1] [2]. Kernel developers usually don't care about POSIX
shell and happily use /bin/bash with all arrays and other crazy bashisms.
There is tool shellcheck, which IMHO has a lot of false positives (I suppose
Cyril agrees with it, as he added checkbashisms to our docs long time ago) and
we both aren't happy about occasional patches which are based on it's output.
See full output below for comparison. E.g. in "In POSIX sh, 'local' is undefined" is
useless, if we decide to trust local, at least for "local msg" i.e. without
assignment. Or other not useful for us:
SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
Not sure about: TST_ARGS="$@":
SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
The only good thing about shellcheck is that it has full shell parser [3],
unlike checkbashisms.
Kind regards,
Petr
[1] https://lwn.net/Articles/343924/
[2] https://lwn.net/Articles/343614/
[3] https://unix.stackexchange.com/questions/667288/checkbashisms-whats-wrong-with-type/667293#comment1257178_667293
$ shellcheck -x tst_ansi_color.sh tst_test.sh
In tst_ansi_color.sh line 9:
local ansi_color_blue='\033[1;34m'
^-------------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_ansi_color.sh line 10:
local ansi_color_green='\033[1;32m'
^--------------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_ansi_color.sh line 11:
local ansi_color_magenta='\033[1;35m'
^----------------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_ansi_color.sh line 12:
local ansi_color_red='\033[1;31m'
^------------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_ansi_color.sh line 13:
local ansi_color_yellow='\033[1;33m'
^---------------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_ansi_color.sh line 16:
TPASS) printf $ansi_color_green;;
^---------------^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
^---------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
TPASS) printf "$ansi_color_green";;
In tst_ansi_color.sh line 17:
TFAIL) printf $ansi_color_red;;
^-------------^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
^-------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
TFAIL) printf "$ansi_color_red";;
In tst_ansi_color.sh line 18:
TBROK) printf $ansi_color_red;;
^-------------^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
^-------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
TBROK) printf "$ansi_color_red";;
In tst_ansi_color.sh line 19:
TWARN) printf $ansi_color_magenta;;
^-----------------^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
^-----------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
TWARN) printf "$ansi_color_magenta";;
In tst_ansi_color.sh line 20:
TINFO) printf $ansi_color_blue;;
^--------------^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
^--------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
TINFO) printf "$ansi_color_blue";;
In tst_ansi_color.sh line 21:
TCONF) printf $ansi_color_yellow;;
^----------------^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
^----------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
TCONF) printf "$ansi_color_yellow";;
In tst_ansi_color.sh line 36:
local color=$?
^---------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_ansi_color.sh line 39:
printf "$2"
^--^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
In tst_test.sh line 29:
local ret=0
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 32:
if [ -n "$TST_DO_CLEANUP" -a -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tst_test.sh line 33:
if type $TST_CLEANUP >/dev/null 2>/dev/null; then
^----------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
if type "$TST_CLEANUP" >/dev/null 2>/dev/null; then
In tst_test.sh line 40:
if [ "$TST_NEEDS_DEVICE" = 1 -a "$TST_DEVICE_FLAG" = 1 ]; then
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tst_test.sh line 46:
if [ "$TST_NEEDS_TMPDIR" = 1 -a -n "$TST_TMPDIR" ]; then
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tst_test.sh line 47:
cd "$LTPROOT"
^-----------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
Did you mean:
cd "$LTPROOT" || exit
In tst_test.sh line 52:
if [ -n "$TST_NEEDS_CHECKPOINTS" -a -f "$LTP_IPC_PATH" ]; then
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tst_test.sh line 53:
rm $LTP_IPC_PATH
^-----------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
rm "$LTP_IPC_PATH"
In tst_test.sh line 70:
if [ $TST_CONF -gt 0 -a $TST_PASS -eq 0 ]; then
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tst_test.sh line 74:
if [ $TST_BROK -gt 0 -o $TST_FAIL -gt 0 -o $TST_WARN -gt 0 ]; then
^-- SC2166: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
^-- SC2166: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
In tst_test.sh line 104:
local res=$1
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 108:
local color=$?
^---------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 112:
printf "$TST_ID $TST_COUNT " >&2
^-------------------^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".
In tst_test.sh line 113:
tst_print_colored $res "$res: " >&2
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
tst_print_colored "$res" "$res: " >&2
In tst_test.sh line 119:
local res=$1
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 133:
local tst_out
^-----------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 135:
tst_out="$(tst_rod $@ 2>&1)"
^-- SC2068: Double quote array expansions to avoid re-splitting elements.
In tst_test.sh line 136:
if [ $? -ne 0 ]; then
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In tst_test.sh line 138:
tst_brk TBROK "$@ failed"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 145:
if [ $? -ne 0 ]; then
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In tst_test.sh line 146:
tst_brk TBROK "$@ failed"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 152:
local fnc="$1"
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 156:
if [ $? -eq 0 ]; then
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In tst_test.sh line 157:
tst_res TPASS "$@ passed as expected"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 160:
$fnc TFAIL "$@ failed unexpectedly"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 167:
local fnc="$1"
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 172:
if [ $? -ne 0 ]; then
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In tst_test.sh line 173:
tst_res TPASS "$@ failed as expected"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 176:
$fnc TFAIL "$@ passed unexpectedly"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 203:
local tst_fun="$1"
^-----------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 204:
local tst_exp=$2
^-----------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 205:
local tst_sec=$(($3 * 1000000))
^-----------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 206:
local tst_delay=1
^-------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 232:
return $tst_exp
^------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
return "$tst_exp"
In tst_test.sh line 242:
return $2
^-- SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
return "$2"
In tst_test.sh line 247:
local msg1="RTNETLINK answers: Function not implemented"
^--------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 248:
local msg2="RTNETLINK answers: Operation not supported"
^--------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 249:
local msg3="RTNETLINK answers: Protocol not supported"
^--------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 250:
local output="$($@ 2>&1 || echo 'LTP_ERR')"
^----------^ SC3043: In POSIX sh, 'local' is undefined.
^----^ SC2155: Declare and assign separately to avoid masking return values.
^-- SC2068: Double quote array expansions to avoid re-splitting elements.
In tst_test.sh line 251:
local msg
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 256:
echo "$output" | grep -q "$msg" && tst_brk TCONF "'$@': $msg"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 259:
tst_brk TBROK "$@ failed: $output"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 285:
local mnt_opt mnt_err
^-------------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 292:
ROD_SILENT mkdir -p $TST_MNTPOINT
^-----------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
ROD_SILENT mkdir -p "$TST_MNTPOINT"
In tst_test.sh line 293:
mount $mnt_opt $TST_DEVICE $TST_MNTPOINT $TST_MNT_PARAMS
^------^ SC2086: Double quote to prevent globbing and word splitting.
^---------^ SC2086: Double quote to prevent globbing and word splitting.
^-----------^ SC2086: Double quote to prevent globbing and word splitting.
^-------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
mount "$mnt_opt" "$TST_DEVICE" "$TST_MNTPOINT" "$TST_MNT_PARAMS"
In tst_test.sh line 294:
local ret=$?
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 307:
local mntpoint="${1:-$TST_MNTPOINT}"
^------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 308:
local i=0
^-----^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 340:
local fs_type=${1:-$TST_FS_TYPE}
^-----------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 341:
local device=${2:-$TST_DEVICE}
^----------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 344:
local fs_opts="$@"
^-----------^ SC3043: In POSIX sh, 'local' is undefined.
^--^ SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
In tst_test.sh line 354:
tst_require_cmds mkfs.$fs_type
^------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
tst_require_cmds mkfs."$fs_type"
In tst_test.sh line 357:
ROD_SILENT mkfs.$fs_type $fs_opts $device
^------^ SC2086: Double quote to prevent globbing and word splitting.
^------^ SC2086: Double quote to prevent globbing and word splitting.
^-----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
ROD_SILENT mkfs."$fs_type" "$fs_opts" "$device"
In tst_test.sh line 366:
local v
^-----^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 372:
[ $? -eq 0 ] || return 1
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In tst_test.sh line 380:
command -v $1 >/dev/null 2>&1
^-- SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
command -v "$1" >/dev/null 2>&1
In tst_test.sh line 385:
local cmd
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 386:
for cmd in $*; do
^-- SC2048: Use "$@" (with quotes) to prevent whitespace problems.
In tst_test.sh line 387:
tst_cmd_available $cmd || tst_brk TCONF "'$cmd' not found"
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
tst_cmd_available "$cmd" || tst_brk TCONF "'$cmd' not found"
In tst_test.sh line 393:
local cmd
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 394:
for cmd in $*; do
^-- SC2048: Use "$@" (with quotes) to prevent whitespace problems.
In tst_test.sh line 395:
if ! tst_cmd_available $cmd; then
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
if ! tst_cmd_available "$cmd"; then
In tst_test.sh line 407:
local drv
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 409:
drv="$(tst_check_drivers $@ 2>&1)"
^-- SC2068: Double quote array expansions to avoid re-splitting elements.
In tst_test.sh line 411:
[ $? -ne 0 ] && tst_brk TCONF "$drv driver not available"
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In tst_test.sh line 446:
local res=$(_tst_resstr)
^-------^ SC3043: In POSIX sh, 'local' is undefined.
^-^ SC2155: Declare and assign separately to avoid masking return values.
In tst_test.sh line 460:
local err="LTP_TIMEOUT_MUL must be number >= 1!"
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 471:
[ "$timeout" -ge 1 ] || tst_brk TBROK "timeout need to be >= 1 ($timeout)"
^------^ SC2154: timeout is referenced but not assigned.
In tst_test.sh line 479:
local i=10
^-----^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 483:
kill -TERM -$pid
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
kill -TERM -"$pid"
In tst_test.sh line 486:
while kill -0 $pid >/dev/null 2>&1 && [ $i -gt 0 ]; do
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
while kill -0 "$pid" >/dev/null 2>&1 && [ $i -gt 0 ]; do
In tst_test.sh line 492:
if kill -0 $pid >/dev/null 2>&1; then
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
if kill -0 "$pid" >/dev/null 2>&1; then
In tst_test.sh line 494:
kill -KILL -$pid
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
kill -KILL -"$pid"
In tst_test.sh line 501:
kill -TERM $_tst_setup_timer_pid 2>/dev/null
^-------------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
kill -TERM "$_tst_setup_timer_pid" 2>/dev/null
In tst_test.sh line 502:
wait $_tst_setup_timer_pid 2>/dev/null
^-------------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
wait "$_tst_setup_timer_pid" 2>/dev/null
In tst_test.sh line 508:
local sleep_pid
^-------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 510:
sleep $sec &
^--^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
sleep "$sec" &
In tst_test.sh line 512:
trap "kill $sleep_pid; exit" TERM
^--------^ SC2064: Use single quotes, otherwise this expands now rather than when signalled.
In tst_test.sh line 531:
local sec=$TST_TIMEOUT
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 533:
local h=$((sec / 3600))
^-----^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 534:
local m=$((sec / 60 % 60))
^-----^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 535:
local s=$((sec % 60))
^-----^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 536:
local pid=$$
^-------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 556:
local _tst_module=$1
^---------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 583:
local pagesize
^------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 587:
if [ $? -ne 0 ]; then
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In tst_test.sh line 597:
local _tst_i
^----------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 598:
local _tst_data
^-------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 599:
local _tst_max
^------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 600:
local _tst_name
^-------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 603:
for _tst_i in $(grep '^[^#]*\bTST_' "$TST_TEST_PATH" | sed 's/.*TST_//; s/[="} \t\/:`].*//'); do
^-- SC2013: To read lines rather than words, pipe/redirect to a 'while read' loop.
In tst_test.sh line 621:
for _tst_i in $(grep '^[^#]*\b_tst_' "$TST_TEST_PATH" | sed 's/.*_tst_//; s/[="} \t\/:`].*//'); do
^-- SC2013: To read lines rather than words, pipe/redirect to a 'while read' loop.
In tst_test.sh line 628:
while getopts ":hi:$TST_OPTS" _tst_name $TST_ARGS; do
^-------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
while getopts ":hi:$TST_OPTS" _tst_name "$TST_ARGS"; do
In tst_test.sh line 650:
tst_require_cmds $TST_NEEDS_CMDS
^-------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
tst_require_cmds "$TST_NEEDS_CMDS"
In tst_test.sh line 651:
tst_require_drivers $TST_NEEDS_DRIVERS
^----------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
tst_require_drivers "$TST_NEEDS_DRIVERS"
In tst_test.sh line 673:
cd "$TST_TMPDIR"
^--------------^ SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
Did you mean:
cd "$TST_TMPDIR" || exit
In tst_test.sh line 681:
if [ ! -b "$TST_DEVICE" -o $? -ne 0 ]; then
^-- SC2166: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined.
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In tst_test.sh line 694:
if type $TST_SETUP >/dev/null 2>/dev/null; then
^--------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
if type "$TST_SETUP" >/dev/null 2>/dev/null; then
In tst_test.sh line 703:
while [ $TST_ITERATIONS -gt 0 ]; do
^-------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
while [ "$TST_ITERATIONS" -gt 0 ]; do
In tst_test.sh line 706:
_tst_max=$(( $(echo $TST_TEST_DATA | tr -cd "$TST_TEST_DATA_IFS" | wc -c) +1))
^------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
_tst_max=$(( $(echo "$TST_TEST_DATA" | tr -cd "$TST_TEST_DATA_IFS" | wc -c) +1))
In tst_test.sh line 708:
_tst_data="$(echo "$TST_TEST_DATA" | cut -d"$TST_TEST_DATA_IFS" -f$_tst_i)"
^-----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
_tst_data="$(echo "$TST_TEST_DATA" | cut -d"$TST_TEST_DATA_IFS" -f"$_tst_i")"
In tst_test.sh line 721:
local _tst_data="$1"
^-------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 722:
local _tst_i
^----------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 725:
for _tst_i in $(seq ${TST_CNT:-1}); do
^-----------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
for _tst_i in $(seq "${TST_CNT:-1}"); do
In tst_test.sh line 726:
if type ${TST_TESTFUNC}1 > /dev/null 2>&1; then
^-------------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
if type "${TST_TESTFUNC}"1 > /dev/null 2>&1; then
In tst_test.sh line 727:
_tst_run_test "$TST_TESTFUNC$_tst_i" $_tst_i "$_tst_data"
^-----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
_tst_run_test "$TST_TESTFUNC$_tst_i" "$_tst_i" "$_tst_data"
In tst_test.sh line 729:
_tst_run_test "$TST_TESTFUNC" $_tst_i "$_tst_data"
^-----^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
_tst_run_test "$TST_TESTFUNC" "$_tst_i" "$_tst_data"
In tst_test.sh line 736:
local _tst_res=$(_tst_resstr)
^------------^ SC3043: In POSIX sh, 'local' is undefined.
^------^ SC2155: Declare and assign separately to avoid masking return values.
In tst_test.sh line 737:
local _tst_fnc="$1"
^------------^ SC3043: In POSIX sh, 'local' is undefined.
In tst_test.sh line 746:
_tst_filename=$(basename $0) || \
^-- SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
_tst_filename=$(basename "$0") || \
In tst_test.sh line 760:
if TST_TEST_PATH=$(command -v $0) 2>/dev/null; then
^-- SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
if TST_TEST_PATH=$(command -v "$0") 2>/dev/null; then
In tst_test.sh line 792:
TST_ARGS="$@"
^--^ SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
In tst_test.sh line 804:
if [ -z "$TST_PRINT_HELP" -a $# -ne "$TST_POS_ARGS" ]; then
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tst_test.sh line 806:
"have ($@) $#, expected ${TST_POS_ARGS}"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
In tst_test.sh line 809:
if [ -z "$TST_PRINT_HELP" -a $# -ne 0 ]; then
^-- SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
In tst_test.sh line 810:
tst_brk TBROK "Unexpected positional arguments '$@'"
^-- SC2145: Argument mixes string and array. Use * or separate argument.
For more information:
https://www.shellcheck.net/wiki/SC2068 -- Double quote array expansions to ...
https://www.shellcheck.net/wiki/SC2145 -- Argument mixes string and array. ...
https://www.shellcheck.net/wiki/SC2048 -- Use "$@" (with quotes) to prevent...
> Joerg
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
2021-09-03 7:43 ` Petr Vorel
@ 2021-09-03 8:10 ` Joerg Vehlow
2021-09-03 8:53 ` Petr Vorel
0 siblings, 1 reply; 20+ messages in thread
From: Joerg Vehlow @ 2021-09-03 8:10 UTC (permalink / raw)
To: ltp
Hi Petr,
On 9/3/2021 9:43 AM, Petr Vorel wrote:
>
>>>> $ checkbashisms testcases/kernel/connectors/pec/cn_pec.sh
>>>> possible bashism in testcases/kernel/connectors/pec/cn_pec.sh line 127
>>>> (should be >word 2>&1):
>>>> ??????????????? done <&${fd_act}
>>>> This one is just a false positive and I have no clue how to prevent this.
>>>> I think the script does not like the <&, but this is posix...
>>> The same here, I'm not sure if it's POSIX. &> definitely is not POSIX.
>>> I remember we were talking about it. Can we avoid it somehow?
>> <&n is the only way to use the file handle n for input. <n would use the
>> file n.
>> See: https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html#tag_02_07_05
>> checkbashisms has no problems if n is a number, not a variable. There is
>> nothing in the standard about n being a variable, but I guess this should be
>> posix as well.
>> Furthermore the suggested fix "(should be >word 2>&1)" clearly shows, that
>> checkbashisms thinks, this is &>.
> Agree, it's very likely checkbashims bug which should be fixed.
>
>> I don't see another way to implement this (but using an implementation in
>> c). And I am not really happy to bend my code around bugs in a tool, that is
>> supposed to ensure better code.
> +1
>> I'd rather try to fix checkbashims in this case. Even the ((-case should be
>> fixed, after checking if it is posix. The suggestion (replace with "$((")
>> indicates at least a bug in the tool.
> I'll try to search for $(( )) in POSIX docs.
>
> What is the outcome? Should I first fix checkbashisms before merging these?
> I suggest to merge it for local checking (similar to checkpatch.pl for C).
> Because it cannot be used in CI right now, not even because these 2 false
> positives, but because not everything has been converted to use new shell API.
I am not sure what the best way is here. I am not against integrating
it, even with the bugs,
just against being "religious" about fixing "bugs" found by it. Of
course that means, no way,
to enable it for ci, at least not without enforcement... But it allows
developers, to quickly run it.
>> To be honest, I am a bit disappointed from this tool. It doesn't seem to be
>> tested very well... Probably barely good enough for scripting in the kernel.
> This tool comes from Debian, written long time ago (2009) for release goal to
> use dash as /bin/sh [1] [2]. Kernel developers usually don't care about POSIX
> shell and happily use /bin/bash with all arrays and other crazy bashisms.
Yeah I mixed up checkbashisms and checkpatch... That's why I pulled
linux into this :)
> There is tool shellcheck, which IMHO has a lot of false positives (I suppose
> Cyril agrees with it, as he added checkbashisms to our docs long time ago) and
> we both aren't happy about occasional patches which are based on it's output.
> See full output below for comparison. E.g. in "In POSIX sh, 'local' is undefined" is
> useless, if we decide to trust local, at least for "local msg" i.e. without
> assignment. Or other not useful for us:
> SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
>
> Not sure about: TST_ARGS="$@":
> SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
>
> The only good thing about shellcheck is that it has full shell parser [3],
> unlike checkbashisms.
Actually scrolling over the results, there are a lot of valid
complaints, e.g. missing quotes.
At least there are no false-positives (as per definition of spellcheck),
as far as I see.
Would it be possible to tailor it for ltp?
Joerg
^ permalink raw reply [flat|nested] 20+ messages in thread
* [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs
2021-09-03 8:10 ` Joerg Vehlow
@ 2021-09-03 8:53 ` Petr Vorel
0 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2021-09-03 8:53 UTC (permalink / raw)
To: ltp
Hi Joerg,
> Hi Petr,
> On 9/3/2021 9:43 AM, Petr Vorel wrote:
> > > > > $ checkbashisms testcases/kernel/connectors/pec/cn_pec.sh
> > > > > possible bashism in testcases/kernel/connectors/pec/cn_pec.sh line 127
> > > > > (should be >word 2>&1):
> > > > > ??????????????? done <&${fd_act}
> > > > > This one is just a false positive and I have no clue how to prevent this.
> > > > > I think the script does not like the <&, but this is posix...
> > > > The same here, I'm not sure if it's POSIX. &> definitely is not POSIX.
> > > > I remember we were talking about it. Can we avoid it somehow?
> > > <&n is the only way to use the file handle n for input. <n would use the
> > > file n.
> > > See: https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html#tag_02_07_05
> > > checkbashisms has no problems if n is a number, not a variable. There is
> > > nothing in the standard about n being a variable, but I guess this should be
> > > posix as well.
> > > Furthermore the suggested fix "(should be >word 2>&1)" clearly shows, that
> > > checkbashisms thinks, this is &>.
> > Agree, it's very likely checkbashims bug which should be fixed.
> > > I don't see another way to implement this (but using an implementation in
> > > c). And I am not really happy to bend my code around bugs in a tool, that is
> > > supposed to ensure better code.
> > +1
> > > I'd rather try to fix checkbashims in this case. Even the ((-case should be
> > > fixed, after checking if it is posix. The suggestion (replace with "$((")
> > > indicates at least a bug in the tool.
> > I'll try to search for $(( )) in POSIX docs.
> > What is the outcome? Should I first fix checkbashisms before merging these?
> > I suggest to merge it for local checking (similar to checkpatch.pl for C).
> > Because it cannot be used in CI right now, not even because these 2 false
> > positives, but because not everything has been converted to use new shell API.
> I am not sure what the best way is here. I am not against integrating it,
> even with the bugs,
Thanks! Feel free to add your ack to patches.
> just against being "religious" about fixing "bugs" found by it.
Agree.
> Of course that means, no way,
> to enable it for ci, at least not without enforcement... But it allows
> developers, to quickly run it.
My short term goal is to run make check-* for those library parts which are
ready for it (both C and shell). That means to list specific targets (some work,
but I'd really have in CI before we convert all tests which allows us to convert
library). We could also add some filtering target to make check, which would be
enabled by environment variable (default off to allow see errors locally,
enabled only for CI).
> > > To be honest, I am a bit disappointed from this tool. It doesn't seem to be
> > > tested very well... Probably barely good enough for scripting in the kernel.
> > This tool comes from Debian, written long time ago (2009) for release goal to
> > use dash as /bin/sh [1] [2]. Kernel developers usually don't care about POSIX
> > shell and happily use /bin/bash with all arrays and other crazy bashisms.
> Yeah I mixed up checkbashisms and checkpatch... That's why I pulled linux
> into this :)
Ah :).
> > There is tool shellcheck, which IMHO has a lot of false positives (I suppose
> > Cyril agrees with it, as he added checkbashisms to our docs long time ago) and
> > we both aren't happy about occasional patches which are based on it's output.
> > See full output below for comparison. E.g. in "In POSIX sh, 'local' is undefined" is
> > useless, if we decide to trust local, at least for "local msg" i.e. without
> > assignment. Or other not useful for us:
> > SC2166: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined.
> > Not sure about: TST_ARGS="$@":
> > SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
> > The only good thing about shellcheck is that it has full shell parser [3],
> > unlike checkbashisms.
> Actually scrolling over the results, there are a lot of valid complaints,
> e.g. missing quotes.
> At least there are no false-positives (as per definition of spellcheck), as
> far as I see.
Yep, I have better impression now than I had before. But integrating it would
require even more fixes than checkbashisms.
> Would it be possible to tailor it for ltp?
Would you prefer to have just shellcheck or use both?
Also both checkbashisms [4] and shellcheck [5] are supported on various distros.
We have vendored checkpatch.pl to allow modifications, the same is for
checkbashisms. Both are perl, which is ok for us as not-required dependency for
building LTP (we already use perl partly for docparse). shellcheck is Haskell
=> we don't want to bring Cabal to LTP build toolchain. But it looks it
shouldn't be needed as it's configurable via .shellcheckrc or in runtime with
--exclude=CODE.
@Cyril?
[4] https://pkgs.org/search/?q=checkbashisms
[5] https://pkgs.org/search/?q=shellcheck
[6] https://github.com/koalaman/shellcheck/tree/master/src/ShellCheck
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [LTP] [PATCH 4/4] doc: Update for vendored checkbashisms.pl
@ 2021-09-09 10:55 ` Petr Vorel
0 siblings, 0 replies; 20+ messages in thread
From: Petr Vorel @ 2021-09-09 10:55 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: Richard Palethorpe, ltp
Hi Cyril,
thanks, merged up to this commit (the 5th one was replaced).
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-09-09 10:55 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 10:37 [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
2021-09-02 10:37 ` [LTP] [PATCH 1/4] doc: Mention make check Petr Vorel
2021-09-02 13:17 ` Cyril Hrubis
2021-09-02 15:46 ` Petr Vorel
2021-09-02 10:37 ` [LTP] [PATCH 2/4] Vendor checkbashisms.pl version 2.20.5 Petr Vorel
2021-09-02 13:25 ` Cyril Hrubis
2021-09-02 10:37 ` [LTP] [PATCH 3/4] rules.mk: Add checkbashisms to 'make check' for *.sh Petr Vorel
2021-09-02 13:27 ` Cyril Hrubis
2021-09-02 15:51 ` Petr Vorel
2021-09-02 10:37 ` [LTP] [PATCH 4/4] doc: Update for vendored checkbashisms.pl Petr Vorel
2021-09-02 13:29 ` Cyril Hrubis
2021-09-09 10:55 ` Petr Vorel
2021-09-09 10:55 ` Petr Vorel
2021-09-02 11:50 ` [LTP] [PATCH 0/4] checkbashisms.pl in make check + fixed docs Petr Vorel
2021-09-02 14:01 ` Joerg Vehlow
2021-09-02 15:09 ` Petr Vorel
2021-09-03 4:28 ` Joerg Vehlow
2021-09-03 7:43 ` Petr Vorel
2021-09-03 8:10 ` Joerg Vehlow
2021-09-03 8:53 ` Petr Vorel
2021-09-02 15:14 ` Petr Vorel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.