| Message ID | 20230119113054.31742-1-atrajeev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] tools/perf/tests: Fix string substitutions in build id test | expand |
From: Athira Rajeev > Sent: 19 January 2023 11:31 ... > diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh > index aaf851108ca3..43e43e131be7 100755 > --- a/tools/perf/tests/shell/buildid.sh > +++ b/tools/perf/tests/shell/buildid.sh > @@ -66,7 +66,7 @@ check() > esac > echo "build id: ${id}" > > - link=${build_id_dir}/.build-id/${id:0:2}/${id:2} > + link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-) > echo "link: ${link}" That is horrid, why not just use valid shell substitutions, eg: id_file=${id#??} id_dir=${id%$id_file} link=$build_id_dir/.build-id/$id_dir/$id_file ... > - check ${@: -1} > + check $last Since this is the end of the shell function you can avoid the eval by doing: shift $(($# - 1)) check $1 or maybe: args="$*" check ${args##* } Those should be ok in all posix shells. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
> On 19-Jan-2023, at 5:32 PM, David Laight <David.Laight@ACULAB.COM> wrote: > > From: Athira Rajeev >> Sent: 19 January 2023 11:31 > ... >> diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh >> index aaf851108ca3..43e43e131be7 100755 >> --- a/tools/perf/tests/shell/buildid.sh >> +++ b/tools/perf/tests/shell/buildid.sh >> @@ -66,7 +66,7 @@ check() >> esac >> echo "build id: ${id}" >> >> - link=${build_id_dir}/.build-id/${id:0:2}/${id:2} >> + link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-) >> echo "link: ${link}" > > That is horrid, why not just use valid shell substitutions, eg: > id_file=${id#??} > id_dir=${id%$id_file} > link=$build_id_dir/.build-id/$id_dir/$id_file > > ... >> - check ${@: -1} >> + check $last > > Since this is the end of the shell function you can avoid the eval > by doing: > shift $(($# - 1)) > check $1 > or maybe: > args="$*" > check ${args##* } > > Those should be ok in all posix shells. > > David > Hi David, Thanks for the review. I will post a V3 addressing these changes. Athira > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
diff --git a/tools/perf/tests/shell/buildid.sh b/tools/perf/tests/shell/buildid.sh index aaf851108ca3..43e43e131be7 100755 --- a/tools/perf/tests/shell/buildid.sh +++ b/tools/perf/tests/shell/buildid.sh @@ -66,7 +66,7 @@ check() esac echo "build id: ${id}" - link=${build_id_dir}/.build-id/${id:0:2}/${id:2} + link=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/$(echo ${id}|cut -c 3-) echo "link: ${link}" if [ ! -h $link ]; then @@ -74,7 +74,7 @@ check() exit 1 fi - file=${build_id_dir}/.build-id/${id:0:2}/`readlink ${link}`/elf + file=${build_id_dir}/.build-id/$(echo ${id}|cut -c 1-2)/`readlink ${link}`/elf echo "file: ${file}" # Check for file permission of original file @@ -130,20 +130,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}" + eval last=\${$#} 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} + check $last - rm -f ${log} + rm -f ${log_out} ${log_err} rm -rf ${build_id_dir} rm -rf ${data} }
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 "cut" command to pick "n" 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: - 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 | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)