Skip to content

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Dec 4, 2020

Just simplifying and reducing the usage of the SCRIPT_NAME indirection, see commit messages.

v1 was fixing 1398cf4 which broke logging by moving the logs of both tests into the same logs/multiple-pipeline/ subdirectory but @xiulipan and @aiChaoSONG said we don't need to move the logs back to where they were, so this PR does not move anything anymore, it's just cleanups now.

For v1 https://sof-ci.01.org/softestpr/PR546/build487/devicetest/ was all green

@marc-hb marc-hb marked this pull request as ready for review December 4, 2020 07:14
@marc-hb marc-hb requested a review from a team as a code owner December 4, 2020 07:14
@marc-hb marc-hb changed the title Reduce usage of SCRIPT_NAME and let wrappers preserve it for the logs/multiple-pipeline-xxx/ directory Reduce usage of SCRIPT_NAME Dec 4, 2020
@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 4, 2020

@marc-hb marc-hb mentioned this pull request Dec 4, 2020
# but the result could not be stored in the array
readarray -t cmd_lst < <(pgrep -P $$ -a|grep -v "$SCRIPT_NAME")
# can't run pgrep in any subshell because the latter would pollute the list
if pgrep -P $$ -a > "$LOG_ROOT/children_left.txt"; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see any need to use a tmp file here, this will bring more trouble with sub-test
why not use readarray -t cmd_lst < <(pgrep -P $$ -a)

And the whole logic is changed to me, the parent will still be killed by itself now.

How is # can't run pgrep in any subshell because the latter would pollute the list take effect here

# NOTICE: already test with $BASHPID:
# it can output the same result of $$
# but the result could not be stored in the array
readarray -t cmd_lst < <(pgrep -P $$ -a|grep -v "$SCRIPT_NAME")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked pgrep -P $$ -a it will show the script it self, how do you remove it from the list

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong, demo:

hostname:~$ pgrep -P $$ -a
hostname:~$ sleep 1000 &
[1] 140297
hostname:~$ 
hostname:~$ ps f
    PID TTY      STAT   TIME COMMAND
 140238 pts/3    Ss     0:00 -bash
 140297 pts/3    S      0:00  \_ sleep 1000
 140298 pts/3    R+     0:00  \_ ps f
 139197 pts/2    Ss+    0:00 -bash
 138772 pts/1    Ss+    0:00 /bin/sh -i
hostname:~$ echo $$
140238
hostname:~$ pgrep -P $$ -a
140297 sleep 1000

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 7, 2020

I do not see any need to use a tmp file here,

The new file is not temporary, it's part of the error logs. Most tests should fail when a process is unexpectedly left behind... but off-topic for now.

How is # can't run pgrep in any subshell because the latter would pollute the list take effect here

Try removing just grep -v "$SCRIPT_NAME" from the existing code (before this PR) and you will see that the <( ) subshell is caught every time. What other reason would the grep -v "$SCRIPT_NAME" be there for?

this will bring more trouble with sub-test

Good point, I think I missed that.

And the whole logic is changed to me

The logic is exactly the same, the only difference is removing the subshell to avoid the awkward filtering.

I checked pgrep -P $$ -a it will show the script it self, how do you remove it from the list

I wonder what you tested because if you run justpgrep -P $$ -a without anything else then it prints nothing. This is expected because a process is not its own parent.

FYI a lot of actual testing went into this PR (except for the subtests, my bad)

@xiulipan
Copy link
Contributor

@marc-hb Try with

cat test_a.sh
#!/bin/bash

aplay -Dhw:0,0 -f dat /dev/zero &
readarray -t cmd_lst < <(pgrep -P $$ -a)

if [ ${#cmd_lst[@]} -gt 0 ]; then
        for line in "${cmd_lst[@]}"
        do
            echo "Catch pid: $line"
            echo "Kill cmd:'${line#* }' by kill -9"
            kill -9 "${line%% *}"
        done
fi

The output is interesting

./test_a.sh
Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
Catch pid: 16164 aplay -Dhw:0,0 -f dat /dev/zero
Kill cmd:'aplay -Dhw:0,0 -f dat /dev/zero' by kill -9
Catch pid: 16165 /bin/bash ./test_a.sh
Kill cmd:'/bin/bash ./test_a.sh' by kill -9
./test_a.sh: line 11: kill: (16165) - No such process

See the log try to kill test_a.sh Kill cmd:'/bin/bash ./test_a.sh' by kill -9
This will why the old code used grep -v to remove the scripts itself.

 cat test_b.sh
#!/bin/bash

aplay -Dhw:0,0 -f dat /dev/zero &
readarray -t cmd_lst < <(pgrep -P $$ -a | grep -v $0)

if [ ${#cmd_lst[@]} -gt 0 ]; then
        for line in "${cmd_lst[@]}"
        do
            echo "Catch pid: $line"
            echo "Kill cmd:'${line#* }' by kill -9"
            kill -9 "${line%% *}"
        done
fi

Above test_b.sh will only kill the aplay.

./test_b.sh
Playing raw data '/dev/zero' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
Catch pid: 16261 aplay -Dhw:0,0 -f dat /dev/zero
Kill cmd:'aplay -Dhw:0,0 -f dat /dev/zero' by kill -9

And that is why I said the logic is different here as I see different behavior with above test scripts.

@plbossart
Copy link
Member

@marc-hb is this still relevant or needs to be closed?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 24, 2021

I've been using this locally and successfully for months. I got confused by some Xiuli's review, will take another look. No one else reviewed unfortunately.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jun 24, 2021

This will why the old code used grep -v to remove the scripts itself.

No, the old code used grep -v to exclude the <( ) SUBSHELL (with the same name), not the main shell which pgrep -P already knows how to exclude. That's why it says "no such process": because the subshell has already completed by the time the main script tries to kill it.

So the only issue left is to make sure sub-tests don't collide their respective children_left.txt files.

You can see the PID is NOT the main shell if you add ps f

--- xiuli-pgrep.sh	2021/06/24 23:45:51	1.1
+++ xiuli-pgrep.sh	2021/06/24 23:46:10
@@ -4,6 +4,8 @@
 aplay -Dhw:0,0 -f dat /dev/zero &
 readarray -t cmd_lst < <(pgrep -P $$ -a)
 
+ps f
+
 if [ ${#cmd_lst[@]} -gt 0 ]; then
         for line in "${cmd_lst[@]}"
         do

You cannot see the subshell <( ) because you cannot ass ps f inside it.

@marc-hb marc-hb added the type:enhancement New framework feature or request label Jul 3, 2021
marc-hb added 2 commits July 29, 2024 17:42
Use a new children_left.txt log file instead.

Using a subshell forced us to filter it out with grep -v $SCRIPT_NAME
which is more complicated, incompatible with exec wrappers like
multiple-pipeline-capture/playback.sh and incompatible with running
concurrent instances.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
The SCRIPT_NAME indirection is not required and confusing.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Apr 22, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:enhancement New framework feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants