[PATCH 1/3] perf tests: Fix record+script_probe_vfs_getname.sh /tmp cleanup

[PATCH 1/3] perf tests: Fix record+script_probe_vfs_getname.sh /tmp cleanup

[James Clark]

The temp files are only cleaned up if the test is not skipped, so delay
making them until after the skip so they don't get left behind in /tmp.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/tests/shell/record+script_probe_vfs_getname.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/shell/record+script_probe_vfs_getname.sh b/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
index bf9e729b3ecf..8d9c04e450ae 100755
--- a/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/record+script_probe_vfs_getname.sh
@@ -15,9 +15,6 @@ skip_if_no_perf_probe || exit 2
 
 . $(dirname $0)/lib/probe_vfs_getname.sh
 
-perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
-file=$(mktemp /tmp/temporary_file.XXXXX)
-
 record_open_file() {
 	echo "Recording open file:"
 	perf record -o ${perfdata} -e probe:vfs_getname\* touch $file
@@ -35,6 +32,9 @@ if [ $err -ne 0 ] ; then
 	exit $err
 fi
 
+perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
+file=$(mktemp /tmp/temporary_file.XXXXX)
+
 record_open_file && perf_script_filenames
 err=$?
 rm -f ${perfdata}
-- 
2.28.0

[PATCH 2/3] perf tests: Fix trace+probe_vfs_getname.sh /tmp cleanup

[James Clark]

The temp file is only cleaned up if the test is not skipped, so delay
making it until after the skip so it doesn't get left behind in /tmp.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/tests/shell/trace+probe_vfs_getname.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/trace+probe_vfs_getname.sh b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
index 3d31c1d560d6..3d60e993d2b8 100755
--- a/tools/perf/tests/shell/trace+probe_vfs_getname.sh
+++ b/tools/perf/tests/shell/trace+probe_vfs_getname.sh
@@ -17,8 +17,6 @@ skip_if_no_perf_trace || exit 2
 
 . $(dirname $0)/lib/probe_vfs_getname.sh
 
-file=$(mktemp /tmp/temporary_file.XXXXX)
-
 trace_open_vfs_getname() {
 	evts=$(echo $(perf list syscalls:sys_enter_open* 2>/dev/null | egrep 'open(at)? ' | sed -r 's/.*sys_enter_([a-z]+) +\[.*$/\1/') | sed 's/ /,/')
 	perf trace -e $evts touch $file 2>&1 | \
@@ -32,6 +30,8 @@ if [ $err -ne 0 ] ; then
 	exit $err
 fi
 
+file=$(mktemp /tmp/temporary_file.XXXXX)
+
 # Do not use whatever ~/.perfconfig file, it may change the output
 # via trace.{show_timestamp,show_prefix,etc}
 export PERF_CONFIG=/dev/null
-- 
2.28.0

[PATCH 3/3] perf tests: Improve temp file cleanup in test_arm_coresight.sh

[James Clark]

Cleanup perf.data.old files which are also dropped by perf, handle
sigint and propagate it to the parent in case the test is run in a bash
while loop and don't create the temp files if the test will be skipped.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/tests/shell/test_arm_coresight.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/shell/test_arm_coresight.sh b/tools/perf/tests/shell/test_arm_coresight.sh
index c9eef0bba6f1..6de53b7ef5ff 100755
--- a/tools/perf/tests/shell/test_arm_coresight.sh
+++ b/tools/perf/tests/shell/test_arm_coresight.sh
@@ -9,8 +9,6 @@
 # SPDX-License-Identifier: GPL-2.0
 # Leo Yan <leo.yan@linaro.org>, 2020
 
-perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
-file=$(mktemp /tmp/temporary_file.XXXXX)
 glb_err=0
 
 skip_if_no_cs_etm_event() {
@@ -22,13 +20,20 @@ skip_if_no_cs_etm_event() {
 
 skip_if_no_cs_etm_event || exit 2
 
+perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
+file=$(mktemp /tmp/temporary_file.XXXXX)
+
 cleanup_files()
 {
 	rm -f ${perfdata}
 	rm -f ${file}
+	rm -f "${perfdata}.old"
+	trap - exit term int
+	kill -2 $$
+	exit $glb_err
 }
 
-trap cleanup_files exit
+trap cleanup_files exit term int
 
 record_touch_file() {
 	echo "Recording trace (only user mode) with path: CPU$2 => $1"
-- 
2.28.0

Re: [PATCH 3/3] perf tests: Improve temp file cleanup in test_arm_coresight.sh

[Ian Rogers]

On Tue, Sep 21, 2021 at 6:10 AM James Clark <james.clark@arm.com> wrote:
>
> Cleanup perf.data.old files which are also dropped by perf, handle
> sigint and propagate it to the parent in case the test is run in a bash
> while loop and don't create the temp files if the test will be skipped.
>
> Signed-off-by: James Clark <james.clark@arm.com>


Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian


>
> ---
>  tools/perf/tests/shell/test_arm_coresight.sh | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/tests/shell/test_arm_coresight.sh b/tools/perf/tests/shell/test_arm_coresight.sh
> index c9eef0bba6f1..6de53b7ef5ff 100755
> --- a/tools/perf/tests/shell/test_arm_coresight.sh
> +++ b/tools/perf/tests/shell/test_arm_coresight.sh
> @@ -9,8 +9,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Leo Yan <leo.yan@linaro.org>, 2020
>
> -perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> -file=$(mktemp /tmp/temporary_file.XXXXX)
>  glb_err=0
>
>  skip_if_no_cs_etm_event() {
> @@ -22,13 +20,20 @@ skip_if_no_cs_etm_event() {
>
>  skip_if_no_cs_etm_event || exit 2
>
> +perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +file=$(mktemp /tmp/temporary_file.XXXXX)
> +
>  cleanup_files()
>  {
>         rm -f ${perfdata}
>         rm -f ${file}
> +       rm -f "${perfdata}.old"
> +       trap - exit term int
> +       kill -2 $$
> +       exit $glb_err
>  }
>
> -trap cleanup_files exit
> +trap cleanup_files exit term int
>
>  record_touch_file() {
>         echo "Recording trace (only user mode) with path: CPU$2 => $1"
> --
> 2.28.0
>

Re: [PATCH 3/3] perf tests: Improve temp file cleanup in test_arm_coresight.sh

[Leo Yan]

On Tue, Sep 21, 2021 at 02:10:09PM +0100, James Clark wrote:
> Cleanup perf.data.old files which are also dropped by perf, handle
> sigint and propagate it to the parent in case the test is run in a bash
> while loop and don't create the temp files if the test will be skipped.
> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/tests/shell/test_arm_coresight.sh | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/tests/shell/test_arm_coresight.sh b/tools/perf/tests/shell/test_arm_coresight.sh
> index c9eef0bba6f1..6de53b7ef5ff 100755
> --- a/tools/perf/tests/shell/test_arm_coresight.sh
> +++ b/tools/perf/tests/shell/test_arm_coresight.sh
> @@ -9,8 +9,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Leo Yan <leo.yan@linaro.org>, 2020
>  
> -perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> -file=$(mktemp /tmp/temporary_file.XXXXX)
>  glb_err=0
>  
>  skip_if_no_cs_etm_event() {
> @@ -22,13 +20,20 @@ skip_if_no_cs_etm_event() {
>  
>  skip_if_no_cs_etm_event || exit 2
>  
> +perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +file=$(mktemp /tmp/temporary_file.XXXXX)
> +
>  cleanup_files()
>  {
>  	rm -f ${perfdata}
>  	rm -f ${file}
> +	rm -f "${perfdata}.old"
> +	trap - exit term int
> +	kill -2 $$

Here it always sends signal SIGINT to current PID with $$, another
choice is to send signal based on the incoming signal type, like below:

  [[ "$1" = "int" ]] || kill -SIGINT $$
  [[ "$1" = "term" ]] || kill -SIGTERM $$

If you think the current change is sufficient to meet the testing
requirement, then this patch would be fine for me either:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

> +	exit $glb_err
>  }
>  
> -trap cleanup_files exit
> +trap cleanup_files exit term int
>  
>  record_touch_file() {
>  	echo "Recording trace (only user mode) with path: CPU$2 => $1"
> -- 
> 2.28.0
> 

Re: [PATCH 3/3] perf tests: Improve temp file cleanup in test_arm_coresight.sh

[James Clark]

On 22/09/2021 12:00, Leo Yan wrote:
> On Tue, Sep 21, 2021 at 02:10:09PM +0100, James Clark wrote:
>> Cleanup perf.data.old files which are also dropped by perf, handle
>> sigint and propagate it to the parent in case the test is run in a bash
>> while loop and don't create the temp files if the test will be skipped.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  tools/perf/tests/shell/test_arm_coresight.sh | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/perf/tests/shell/test_arm_coresight.sh b/tools/perf/tests/shell/test_arm_coresight.sh
>> index c9eef0bba6f1..6de53b7ef5ff 100755
>> --- a/tools/perf/tests/shell/test_arm_coresight.sh
>> +++ b/tools/perf/tests/shell/test_arm_coresight.sh
>> @@ -9,8 +9,6 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  # Leo Yan <leo.yan@linaro.org>, 2020
>>  
>> -perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
>> -file=$(mktemp /tmp/temporary_file.XXXXX)
>>  glb_err=0
>>  
>>  skip_if_no_cs_etm_event() {
>> @@ -22,13 +20,20 @@ skip_if_no_cs_etm_event() {
>>  
>>  skip_if_no_cs_etm_event || exit 2
>>  
>> +perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
>> +file=$(mktemp /tmp/temporary_file.XXXXX)
>> +
>>  cleanup_files()
>>  {
>>  	rm -f ${perfdata}
>>  	rm -f ${file}
>> +	rm -f "${perfdata}.old"
>> +	trap - exit term int
>> +	kill -2 $$
> 
> Here it always sends signal SIGINT to current PID with $$, another
> choice is to send signal based on the incoming signal type, like below:
> 
>   [[ "$1" = "int" ]] || kill -SIGINT $$
>   [[ "$1" = "term" ]] || kill -SIGTERM $$

Yes I thought that this might be an issue, but I tested it in a few different
scenarios. Especially when running it under the normal ./perf test command and
it didn't seem to cause an issue whether it passed or failed. So I'm not sure
if it's worth changing or not. Maybe it is in case it gets copy pasted into
another shell test?

James

> 
> If you think the current change is sufficient to meet the testing
> requirement, then this patch would be fine for me either:
> 
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> 
>> +	exit $glb_err
>>  }
>>  
>> -trap cleanup_files exit
>> +trap cleanup_files exit term int
>>  
>>  record_touch_file() {
>>  	echo "Recording trace (only user mode) with path: CPU$2 => $1"
>> -- 
>> 2.28.0
>>

Re: [PATCH 3/3] perf tests: Improve temp file cleanup in test_arm_coresight.sh

[Leo Yan]

Hi James,

On Wed, Sep 22, 2021 at 02:40:54PM +0100, James Clark wrote:

[...]

> >>  cleanup_files()
> >>  {
> >>  	rm -f ${perfdata}
> >>  	rm -f ${file}
> >> +	rm -f "${perfdata}.old"
> >> +	trap - exit term int
> >> +	kill -2 $$
> > 
> > Here it always sends signal SIGINT to current PID with $$, another
> > choice is to send signal based on the incoming signal type, like below:
> > 
> >   [[ "$1" = "int" ]] || kill -SIGINT $$
> >   [[ "$1" = "term" ]] || kill -SIGTERM $$
> 
> Yes I thought that this might be an issue, but I tested it in a few different
> scenarios. Especially when running it under the normal ./perf test command and
> it didn't seem to cause an issue whether it passed or failed. So I'm not sure
> if it's worth changing or not. Maybe it is in case it gets copy pasted into
> another shell test?

I think it's not very necessary to send signal again with command
"kill -2 $$" at here.

"kill -2 $$" sends the signal to the shell process itself rather than
propagating signal to its parent process.  And the up level's script
should can detect an error with the returned exit code.

So below change should be sufficient?

cleanup_files()
{
	rm -f ${perfdata}
	rm -f ${file}
+       rm -f "${perfdata}.old"
+       exit $glb_err
}

Sorry if I miss anything at here and cause noise.

Thanks,
Leo

Re: [PATCH 3/3] perf tests: Improve temp file cleanup in test_arm_coresight.sh

[James Clark]

On 22/09/2021 15:08, Leo Yan wrote:
> Hi James,
> 
> On Wed, Sep 22, 2021 at 02:40:54PM +0100, James Clark wrote:
> 
> [...]
> 
>>>>  cleanup_files()
>>>>  {
>>>>  	rm -f ${perfdata}
>>>>  	rm -f ${file}
>>>> +	rm -f "${perfdata}.old"
>>>> +	trap - exit term int
>>>> +	kill -2 $$
>>>
>>> Here it always sends signal SIGINT to current PID with $$, another
>>> choice is to send signal based on the incoming signal type, like below:
>>>
>>>   [[ "$1" = "int" ]] || kill -SIGINT $$
>>>   [[ "$1" = "term" ]] || kill -SIGTERM $$
>>
>> Yes I thought that this might be an issue, but I tested it in a few different
>> scenarios. Especially when running it under the normal ./perf test command and
>> it didn't seem to cause an issue whether it passed or failed. So I'm not sure
>> if it's worth changing or not. Maybe it is in case it gets copy pasted into
>> another shell test?
> 
> I think it's not very necessary to send signal again with command
> "kill -2 $$" at here.
> 
> "kill -2 $$" sends the signal to the shell process itself rather than
> propagating signal to its parent process.  And the up level's script
> should can detect an error with the returned exit code.
> 
> So below change should be sufficient?
> 
> cleanup_files()
> {
> 	rm -f ${perfdata}
> 	rm -f ${file}
> +       rm -f "${perfdata}.old"
> +       exit $glb_err
> }
> 
> Sorry if I miss anything at here and cause noise.

The problem with not re-sending the sigint is that if you want to run the
script in a bash while loop like:

  while ! tests/shell/test_arm_coresight.sh; do echo loop; done

Then it's impossible to exit with Ctrl-C and delete the temp files at the
same time. It exits if we don't trap sigint like it is at the moment, but
then it leaves the temporary files. This change is so we can have both
behaviours of Ctrl-C in a loop and keep the cleanup working.


> 
> Thanks,
> Leo
> 

Re: [PATCH 3/3] perf tests: Improve temp file cleanup in test_arm_coresight.sh

[Leo Yan]

On Wed, Sep 22, 2021 at 05:49:55PM +0100, James Clark wrote:

[...]

> > So below change should be sufficient?
> > 
> > cleanup_files()
> > {
> > 	rm -f ${perfdata}
> > 	rm -f ${file}
> > +       rm -f "${perfdata}.old"
> > +       exit $glb_err
> > }
> > 
> > Sorry if I miss anything at here and cause noise.
> 
> The problem with not re-sending the sigint is that if you want to run the
> script in a bash while loop like:
> 
>   while ! tests/shell/test_arm_coresight.sh; do echo loop; done
> 
> Then it's impossible to exit with Ctrl-C and delete the temp files at the
> same time. It exits if we don't trap sigint like it is at the moment, but
> then it leaves the temporary files. This change is so we can have both
> behaviours of Ctrl-C in a loop and keep the cleanup working.

Okay, I cannot think out better idea to handle this test case;
so current patch is fine for me.  Thanks for explanation.

Thanks,
Leo

Re: [PATCH 1/3] perf tests: Fix record+script_probe_vfs_getname.sh /tmp cleanup

[Arnaldo Carvalho de Melo]

Em Tue, Sep 21, 2021 at 11:07:50PM -0700, Ian Rogers escreveu:
> On Tue, Sep 21, 2021 at 6:10 AM James Clark <james.clark@arm.com> wrote:
> 
> > The temp files are only cleaned up if the test is not skipped, so delay
> > making them until after the skip so they don't get left behind in /tmp.
> >
> > Signed-off-by: James Clark <james.clark@arm.com>
> >
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied the three patches in this series.

- Arnaldo