Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce usage of SCRIPT_NAME #546

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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
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 <[email protected]>
The SCRIPT_NAME indirection is not required and confusing.

Signed-off-by: Marc Herbert <[email protected]>
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