Message ID | 20230119142719.32628-1-atrajeev@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | [V3] tools/perf/tests: Fix string substitutions in build id test | expand |
From: Athira Rajeev > Sent: 19 January 2023 14:27 ... > The test script "tests/shell/buildid.sh" uses some of the > string substitution ways which are supported in bash, but not in > "sh" or other shells. Above error on line number 69 that reports > "Bad substitution" is: Looks better - assuming it works :-) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Em Thu, Jan 19, 2023 at 03:49:15PM +0000, David Laight escreveu: > From: Athira Rajeev > > Sent: 19 January 2023 14:27 > ... > > The test script "tests/shell/buildid.sh" uses some of the > > string substitution ways which are supported in bash, but not in > > "sh" or other shells. Above error on line number 69 that reports > > "Bad substitution" is: > > Looks better - assuming it works :-) :-) Can I take this as an: Acked-by: David Laight <David.Laight@ACULAB.COM> ? - Arnaldo
Environment with /bin/sh # readlink -f $(which sh) /bin/dash Running perf test from tmp.perf/urgent # ./perf test -v "build id cache operations" 73: build id cache operations : --- start --- test child forked, pid 71063 WARNING: wine not found. PE binaries will not be run. test binaries: /tmp/perf.ex.SHA1.RNm /tmp/perf.ex.MD5.Flx ./tests/shell/../pe-file.exe DEBUGINFOD_URLS= Adding 4abd406f041feb4f10ecde3fc30fd0639e1a91cb /tmp/perf.ex.SHA1.RNm: Ok build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb ./tests/shell/buildid.sh: 69: ./tests/shell/buildid.sh: Bad substitution test child finished with -2 ---- end ---- After applying the patch in same environment, error is not seen and test runs fine. Tested the same patch in bash as well. # readlink -f $(which sh) /usr/bin/bash The test works fine with the changes. Tested-by: Disha Goel<disgoel@linux.ibm.com> On 1/19/23 7:57 PM, Athira Rajeev wrote: > The perf test named “build id cache operations” skips with below > error on some distros: > > <<>> > 78: build id cache operations : > test child forked, pid 111101 > WARNING: wine not found. PE binaries will not be run. > test binaries: /tmp/perf.ex.SHA1.PKz /tmp/perf.ex.MD5.Gt3 ./tests/shell/../pe-file.exe > DEBUGINFOD_URLS= > Adding 4abd406f041feb4f10ecde3fc30fd0639e1a91cb /tmp/perf.ex.SHA1.PKz: Ok > build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb > ./tests/shell/buildid.sh: 69: ./tests/shell/buildid.sh: Bad substitution > test child finished with -2 > build id cache operations: Skip > <<>> > > The test script "tests/shell/buildid.sh" uses some of the > string substitution ways which are supported in bash, but not in > "sh" or other shells. Above error on line number 69 that reports > "Bad substitution" is: > > <<>> > link=${build_id_dir}/.build-id/${id:0:2}/${id:2} > <<>> > > Here the way of getting first two characters from id ie, > ${id:0:2} and similarly expressions like ${id:2} is not > recognised in "sh". So the line errors and instead of > hitting failure, the test gets skipped as shown in logs. > So the syntax issue causes test not to be executed in > such cases. Similarly usage : "${@: -1}" [ to pick last > argument passed to a function] in “test_record” doesn’t > work in all distros. > > Fix this by using alternative way with shell substitution > to pick required characters from the string. Also fix the > usage of “${@: -1}” to work in all cases. > > Another usage in “test_record” is: > <<>> > ${perf} record --buildid-all -o ${data} $@ &> ${log} > <<>> > > This causes the perf record to start in background and > Results in the data file not being created by the time > "check" function is invoked. Below log shows perf record > result getting displayed after the call to "check" function. > > <<>> > running: perf record /tmp/perf.ex.SHA1.EAU > build id: 4abd406f041feb4f10ecde3fc30fd0639e1a91cb > link: /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb > failed: link /tmp/perf.debug.mLT/.build-id/4a/bd406f041feb4f10ecde3fc30fd0639e1a91cb does not exist > test child finished with -1 > build id cache operations: FAILED! > root@machine:~/athira/linux/tools/perf# Couldn't synthesize bpf events. > [ perf record: Woken up 1 times to write data ] > [ perf record: Captured and wrote 0.010 MB /tmp/perf.data.bFF ] > <<>> > > Fix this by redirecting output instead of using “&” which > starts the command in background. > > Signed-off-by: Athira Rajeev<atrajeev@linux.vnet.ibm.com> > Acked-by: Ian Rogers<irogers@google.com> > --- > Changelog: > From v2 -> v3 > - Addressed review comments from David Laight > for using shell substitutions. > > From v1 -> v2 > - Added Acked-by from Ian. > - Rebased to tmp.perf/urgent of: > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git > > tools/perf/tests/shell/buildid.sh | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh > index aaf851108ca3..0ce22ea0a7f1 100755 > --- a/tools/perf/tests/shell/buildid.sh > +++ b/tools/perf/tests/shell/buildid.sh > @@ -66,7 +66,9 @@ check() > esac > echo "build id: ${id}" > > - link=${build_id_dir}/.build-id/${id:0:2}/${id:2} > + id_file=${id#??} > + id_dir=${id%$id_file} > + link=$build_id_dir/.build-id/$id_dir/$id_file > echo "link: ${link}" if [ ! -h $link ]; then @@ -74,7 +76,7 @@ check() exit 1 fi - > file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf + > file=${build_id_dir}/.build-id/$id_dir/`readlink ${link}`/elf echo "file: ${file}" > > # Check for file permission of original file > @@ -130,20 +132,22 @@ test_record() > { > data=$(mktemp /tmp/perf.data.XXX) > build_id_dir=$(mktemp -d /tmp/perf.debug.XXX) > - log=$(mktemp /tmp/perf.log.XXX) > + log_out=$(mktemp /tmp/perf.log.out.XXX) > + log_err=$(mktemp /tmp/perf.log.err.XXX) > perf="perf --buildid-dir ${build_id_dir}" > > echo "running: perf record $@" > - ${perf} record --buildid-all -o ${data} $@ &> ${log} > + ${perf} record --buildid-all -o ${data} $@ 1>${log_out} 2>${log_err} > if [ $? -ne 0 ]; then > echo "failed: record $@" > - echo "see log: ${log}" > + echo "see log: ${log_err}" > exit 1 > fi > > - check ${@: -1} > + args="$*" > + check ${args##* } > > - rm -f ${log} > + rm -f ${log_out} ${log_err} > rm -rf ${build_id_dir} > rm -rf ${data} > }
From: Arnaldo Carvalho de Melo > Sent: 19 January 2023 17:00 > > Em Thu, Jan 19, 2023 at 03:49:15PM +0000, David Laight escreveu: > > From: Athira Rajeev > > > Sent: 19 January 2023 14:27 > > ... > > > The test script "tests/shell/buildid.sh" uses some of the > > > string substitution ways which are supported in bash, but not in > > > "sh" or other shells. Above error on line number 69 that reports > > > "Bad substitution" is: > > > > Looks better - assuming it works :-) > > :-) > > Can I take this as an: > > Acked-by: David Laight <David.Laight@ACULAB.COM> I'm not sure that is worth anything. You can add a Reviewed-by David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Em Fri, Jan 20, 2023 at 08:51:59AM +0000, David Laight escreveu: > From: Arnaldo Carvalho de Melo > > Sent: 19 January 2023 17:00 > > > > Em Thu, Jan 19, 2023 at 03:49:15PM +0000, David Laight escreveu: > > > From: Athira Rajeev > > > > Sent: 19 January 2023 14:27 > > > ... > > > > The test script "tests/shell/buildid.sh" uses some of the > > > > string substitution ways which are supported in bash, but not in > > > > "sh" or other shells. Above error on line number 69 that reports > > > > "Bad substitution" is: > > > > > > Looks better - assuming it works :-) > > > > :-) > > > > Can I take this as an: > > > > Acked-by: David Laight <David.Laight@ACULAB.COM> > > I'm not sure that is worth anything. > You can add a Reviewed-by Thanks, I'll then add: Reviewed-by: David Laight <David.Laight@ACULAB.COM> - Arnaldo
diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh index aaf851108ca3..0ce22ea0a7f1 100755 --- a/tools/perf/tests/shell/buildid.sh +++ b/tools/perf/tests/shell/buildid.sh @@ -66,7 +66,9 @@ check() esac echo "build id: ${id}" - link=${build_id_dir}/.build-id/${id:0:2}/${id:2} + id_file=${id#??} + id_dir=${id%$id_file} + link=$build_id_dir/.build-id/$id_dir/$id_file echo "link: ${link}" if [ ! -h $link ]; then @@ -74,7 +76,7 @@ check() exit 1 fi - file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf + file=${build_id_dir}/.build-id/$id_dir/`readlink ${link}`/elf echo "file: ${file}" # Check for file permission of original file @@ -130,20 +132,22 @@ test_record() { data=$(mktemp /tmp/perf.data.XXX) build_id_dir=$(mktemp -d /tmp/perf.debug.XXX) - log=$(mktemp /tmp/perf.log.XXX) + log_out=$(mktemp /tmp/perf.log.out.XXX) + log_err=$(mktemp /tmp/perf.log.err.XXX) perf="perf --buildid-dir ${build_id_dir}" echo "running: perf record $@" - ${perf} record --buildid-all -o ${data} $@ &> ${log} + ${perf} record --buildid-all -o ${data} $@ 1>${log_out} 2>${log_err} if [ $? -ne 0 ]; then echo "failed: record $@" - echo "see log: ${log}" + echo "see log: ${log_err}" exit 1 fi - check ${@: -1} + args="$*" + check ${args##* } - rm -f ${log} + rm -f ${log_out} ${log_err} rm -rf ${build_id_dir} rm -rf ${data} }