On 20-Jul-2023, at 10:48 AM, kajoljain [off-list ref] wrote:



On 7/20/23 10:42, Athira Rajeev wrote:

quoted

quoted

On 19-Jul-2023, at 11:16 PM, Ian Rogers [off-list ref] wrote:

On Tue, Jul 18, 2023 at 11:17 PM kajoljain [off-list ref] wrote:

quoted

Hi,

Looking for review comments on this patchset.

Thanks,
Kajol Jain


On 7/9/23 23:57, Athira Rajeev wrote:

quoted

Patchset covers a set of fixes for coding/formatting issues observed while
running shellcheck tool on the perf shell scripts.

This cleanup is a pre-requisite to include a build option for shellcheck
discussed here: https://www.spinics.net/lists/linux-perf-users/msg25553.html
First set of patches were posted here:
https://lore.kernel.org/linux-perf-users/53B7D823-1570-4289-A632-2205EE2B522C@linux.vnet.ibm.com/T/#t (local)

This patchset covers remaining set of shell scripts which needs
fix. Patch 1 is resubmission of patch 6 from the initial series.
Patch 15, 16 and 22 touches code from tools/perf/trace/beauty.
Other patches are fixes for scripts from tools/perf/tests.

The shellcheck is run for severity level for errors and warnings.
Command used:

# for F in $(find tests/shell/ -perm -o=x -name '*.sh'); do shellcheck -S warning $F; done
# echo $?
0

I don't see anything objectionable in the changes so for the series:
Acked-by: Ian Rogers <irogers@google.com>

Some thoughts:
- Adding "#!/bin/bash" to scripts in tools/perf/tests/lib - I think
we didn't do this to avoid these being included as tests. There are
now extra checks when finding shell tests, so I can imagine doing this
isn't a regression but just a heads up.
- I think James' comment was addressed:
https://lore.kernel.org/linux-perf-users/334989bf-5501-494c-f246-81878fd2fed8@arm.com/ (local)
- Why aren't these changes being mailed to LKML? The wider community
on LKML have thoughts on shell scripts, plus it makes the changes miss
my mail filters.
- Can we automate this testing into the build? For example, following
a similar kernel build pattern we run a python test and make the log
output a requirement here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/Build?h=perf-tools-next#n30
 I think we can translate:
for F in $(find tests/shell/ -perm -o=x -name '*.sh'); do shellcheck
-S warning $F; done
 into a rule in make for log files that are then a dependency on the
perf binary. We can then parallel shellcheck during the build and
avoid regressions. We probably need a CONFIG_SHELLCHECK feature check
in the build to avoid not having shellcheck breaking the build.

Hi Ian

Thanks for the comments.
Yes, next step after this is to include build option for shellcheck by updating Makefile.
We will surely get into that build option enablement patch once we have all these corrections in place.

Thanks
Athira

quoted

Hi Ian,
  Thanks for reviewing the patches. As athira mentioned our next is to
include build option. So, we will work on it next once all the
correction done.

Thanks,
Kajol Jain

Hi Arnaldo,  Namhyung

Can you have this patchset applied along with Acked-by from Ian ?
Our next step is to add a build option for shellcheck by updating Makefile and will be working on that.

Thanks
Athira 

quoted

quoted

Thanks,
Ian

quoted

quoted

Changelog:
v1 -> v2:
- Rebased on top of perf-tools-next from:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=perf-tools-next

- Fixed shellcheck errors and warnings reported for newly
  added changes from perf-tools-next branch

- Addressed review comment from James clark for patch
  number 13 from V1. The changes in patch 13 were not necessary
  since the file "tests/shell/lib/coresight.sh" is sourced from
  other test files.

Akanksha J N (1):
tools/perf/tests: Fix shellcheck warnings for
  trace+probe_vfs_getname.sh

Athira Rajeev (14):
tools/perf/tests: fix test_arm_spe_fork.sh signal case issues
tools/perf/tests: Fix unused variable references in
  stat+csv_summary.sh testcase
tools/perf/tests: fix shellcheck warning for
  test_perf_data_converter_json.sh testcase
tools/perf/tests: Fix shellcheck issue for stat_bpf_counters.sh
  testcase
tools/perf/tests: Fix shellcheck issues in
  tests/shell/stat+shadow_stat.sh tetscase
tools/perf/tests: Fix shellcheck warnings for
  thread_loop_check_tid_10.sh
tools/perf/tests: Fix shellcheck warnings for unroll_loop_thread_10.sh
tools/perf/tests: Fix shellcheck warnings for lib/probe_vfs_getname.sh
tools/perf/tests: Fix the shellcheck warnings in lib/waiting.sh
tools/perf/trace: Fix x86_arch_prctl.sh to address shellcheck warnings
tools/perf/arch/x86: Fix syscalltbl.sh to address shellcheck warnings
tools/perf/tests/shell: Fix the shellcheck warnings in
  record+zstd_comp_decomp.sh
tools/perf/tests/shell: Fix shellcheck warning for stat+std_output.sh
  testcase
tools/perf/tests: Fix shellcheck warning for stat+std_output.sh
  testcase

Kajol Jain (11):
tools/perf/tests: Fix shellcheck warning for probe_vfs_getname.sh
  testcase
tools/perf/tests: Fix shellcheck warning for record_offcpu.sh testcase
tools/perf/tests: Fix shellcheck issue for lock_contention.sh testcase
tools/perf/tests: Fix shellcheck issue for stat_bpf_counters_cgrp.sh
  testcase
tools/perf/tests: Fix shellcheck warning for asm_pure_loop.sh shell
  script
tools/perf/tests: Fix shellcheck warning for memcpy_thread_16k_10.sh
  shell script
tools/perf/tests: Fix shellcheck warning for probe.sh shell script
tools/perf/trace: Fix shellcheck issue for arch_errno_names.sh script
tools/perf: Fix shellcheck issue for check-headers.sh script
tools/shell/coresight: Fix shellcheck warning for
  thread_loop_check_tid_2.sh shell script
tools/perf/tests/shell/lib: Fix shellcheck warning for stat_output.sh
  shell script

.../arch/x86/entry/syscalls/syscalltbl.sh     |  2 +-
tools/perf/check-headers.sh                   |  6 ++--
.../tests/shell/coresight/asm_pure_loop.sh    |  2 +-
.../shell/coresight/memcpy_thread_16k_10.sh   |  2 +-
.../coresight/thread_loop_check_tid_10.sh     |  2 +-
.../coresight/thread_loop_check_tid_2.sh      |  2 +-
.../shell/coresight/unroll_loop_thread_10.sh  |  2 +-
tools/perf/tests/shell/lib/probe.sh           |  1 +
.../perf/tests/shell/lib/probe_vfs_getname.sh |  5 ++--
tools/perf/tests/shell/lib/stat_output.sh     |  1 +
tools/perf/tests/shell/lib/waiting.sh         |  1 +
tools/perf/tests/shell/lock_contention.sh     | 12 ++++----
tools/perf/tests/shell/probe_vfs_getname.sh   |  4 +--
.../tests/shell/record+zstd_comp_decomp.sh    | 14 +++++-----
tools/perf/tests/shell/record_offcpu.sh       |  6 ++--
tools/perf/tests/shell/stat+csv_output.sh     |  2 +-
tools/perf/tests/shell/stat+csv_summary.sh    |  4 +--
tools/perf/tests/shell/stat+shadow_stat.sh    |  4 +--
tools/perf/tests/shell/stat+std_output.sh     |  3 +-
tools/perf/tests/shell/stat_bpf_counters.sh   |  4 +--
.../tests/shell/stat_bpf_counters_cgrp.sh     | 28 ++++++++-----------
tools/perf/tests/shell/test_arm_spe_fork.sh   |  2 +-
.../shell/test_perf_data_converter_json.sh    |  2 +-
.../tests/shell/trace+probe_vfs_getname.sh    |  6 ++--
tools/perf/trace/beauty/arch_errno_names.sh   | 15 ++++------
tools/perf/trace/beauty/x86_arch_prctl.sh     |  6 ++--
26 files changed, 67 insertions(+), 71 deletions(-)