[LTP] [PATCH v2] pec: Fix multiple event test

[LTP] [PATCH v2] pec: Fix multiple event test

[Joerg Vehlow]

From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

The test case for NUM_EVENTS != 1 was not correct at all.
Now the test looks for each event recorded by the generator
in the correct order in the listener's log.

Additionally the test is parameterized, to the the number of generated events
and the default is now set to 10.

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 testcases/kernel/connectors/pec/cn_pec.sh     | 100 +++++++++++++++---
 .../kernel/connectors/pec/event_generator.c   |  54 +++++-----
 2 files changed, 116 insertions(+), 38 deletions(-)

diff --git a/testcases/kernel/connectors/pec/cn_pec.sh b/testcases/kernel/connectors/pec/cn_pec.sh
index 4ab84b4fa..0acb7271d 100755
--- a/testcases/kernel/connectors/pec/cn_pec.sh
+++ b/testcases/kernel/connectors/pec/cn_pec.sh
@@ -9,16 +9,52 @@
 # Process event connector is a netlink connector that reports process events
 # to userspace. It sends events such as fork, exec, id change and exit.
 
+TST_OPTS="n:"
 TST_SETUP=setup
 TST_TESTFUNC=test
+TST_PARSE_ARGS=parse_args
+TST_USAGE=usage
 TST_NEEDS_ROOT=1
 TST_NEEDS_TMPDIR=1
 TST_TEST_DATA="fork exec exit uid gid"
 
-NUM_EVENTS=1
-
 . tst_test.sh
 
+num_events=10
+
+usage()
+{
+	cat << EOF
+usage: $0 [-n <nevents>]
+
+OPTIONS
+-n      The number of evetns to generate per test (default 10)
+EOF
+}
+
+parse_args()
+{
+	case $1 in
+	n) num_events=$2;;
+	esac
+}
+
+# Find a free file handle
+free_fd()
+{
+	local fd
+
+	for fd in $(seq 200); do
+		# Sapwn a new sh, because redirecting to a non existing file handle
+		# will trigger a syntax error.
+		/bin/sh -c ": 2>/dev/null >&$fd || : 2>/dev/null <&$fd" 2>/dev/null
+		if [ $? -eq 2 ]; then
+			echo $fd
+			return
+		fi
+	done
+}
+
 setup()
 {
 	if ! grep -q cn_proc /proc/net/connector; then
@@ -31,7 +67,9 @@ setup()
 test()
 {
 	local event=$2
-	local expected_events lis_rc pid
+	local expected_events lis_rc pid fd_act failed act_nevents exp act
+
+	tst_res TINFO "Testing $2 event (nevents=$num_events)"
 
 	pec_listener >lis_$event.log 2>lis_$event.err &
 	pid=$!
@@ -39,28 +77,66 @@ test()
 	tst_sleep 100ms
 
 	# generator must be in PATH
-	ROD event_generator -n $NUM_EVENTS -e $event \>gen_$event.log 2\>gen_$event.err
+	ROD event_generator -n $num_events -e $event \>gen_$event.log 2\>gen_$event.err
 
-	# sleep until pec_listener has seen and handled all of the generated events
-	tst_sleep 100ms
 	kill -s INT $pid 2> /dev/null
 	wait $pid
 	lis_rc=$?
 
 	if [ ! -s gen_$event.log ]; then
-		tst_brk TBROK "failed to generate process events"
+		tst_brk TBROK "failed to generate process events: $(cat gen_$event.err)"
 	fi
 
 	if [ $lis_rc -ne 0 ]; then
 		tst_brk TBROK "failed to execute the listener: $(cat lis_$event.err)"
 	fi
 
-	expected_events="$(cat gen_$event.log)"
-	if grep -q "$expected_events" lis_$event.log; then
-		tst_res TPASS "$event detected by listener"
+	# The listener writes the same messages as the generator, but it can
+	# also see more events (e.g. for testing exit, a fork is generated).
+	# So: The events generated by the generator have to be in the same order
+	# as the events printed by the listener, but my interleaved with other
+	# messages. To correctly compare them, we have to open both logs
+	# and iterate over both of them at the same time, skipping messages
+	# in the listener log, that are not of interest.
+	# Because some messages may be multiple times in the listener log,
+	# we have to open it only once!
+	# This however does not check, if the listener sees more messages,
+	# than expected.
+
+	fd_act=$(free_fd)
+	[ -z "$fd_act" ] && tst_brk TBROK "No free filehandle found"
+	eval "exec ${fd_act}<lis_$event.log"
+
+	failed=0
+	act_nevents=0
+	while read -r exp; do
+		local found=0
+		act_nevents=$((act_nevents + 1))
+		while read -r act; do
+			if [ "$exp" = "$act" ]; then
+				found=1
+				break
+			fi
+		done <&${fd_act}
+		if [ $found -ne 1 ]; then
+			failed=1
+			tst_res TFAIL "Event was not detected by the event listener: $exp"
+			break
+		fi
+	done <gen_$event.log
+
+	eval "exec ${fd_act}<&-"
+
+	if [ $failed -eq 0 ]; then
+		if [ $act_nevents -ne $num_events ]; then
+			tst_res TBROK "Expected $num_events, but $act_nevents generated"
+		else
+			tst_res TPASS "All events detected"
+		fi
 	else
-		tst_res TFAIL "$event not detected by listener"
-	fi
+		# TFAIL message is already printed in the loop above
+		cat lis_$event.log
+ 	fi
 }
 
 tst_run
diff --git a/testcases/kernel/connectors/pec/event_generator.c b/testcases/kernel/connectors/pec/event_generator.c
index 0a90b91c4..62e341268 100644
--- a/testcases/kernel/connectors/pec/event_generator.c
+++ b/testcases/kernel/connectors/pec/event_generator.c
@@ -93,16 +93,8 @@ static void gen_exec(void)
  */
 static inline void gen_fork(void)
 {
-	pid_t pid;
-	int status;
-
-	pid = SAFE_FORK();
-	if (pid == 0) {
-		printf("fork parent: %d, child: %d\n", getppid(), getpid());
-		exit(0);
-	} else {		/* Parent should wait for the child */
-		wait(&status);
-	}
+	/* The actual fork is already done in main */
+	printf("fork parent: %d, child: %d\n", getppid(), getpid());
 }
 
 /**
@@ -110,16 +102,10 @@ static inline void gen_fork(void)
  */
 static inline void gen_exit(void)
 {
-	pid_t pid;
-	int status;
-
-	pid = SAFE_FORK();
-	if (pid == 0) {
-		printf("exit pid: %d exit_code: %d\n", getpid(), 0);
-		exit(0);
-	} else {
-		wait(&status);
-	}
+	/* exit_signal will always be SIGCHLD, if the process terminates cleanly */
+	printf("exit pid: %d exit_code: %d exit_signal: %d\n",
+	       getpid(), 0, SIGCHLD);
+	/* exit is called by main already */
 }
 
 /*
@@ -128,7 +114,7 @@ static inline void gen_exit(void)
 static inline void gen_uid(void)
 {
 	SAFE_SETUID(ltp_uid);
-	printf("uid pid: %d euid: %d\n", getpid(), ltp_uid);
+	printf("uid pid: %d euid: %d ruid: %d\n", getpid(), ltp_uid, ltp_uid);
 }
 
 /*
@@ -137,7 +123,7 @@ static inline void gen_uid(void)
 static inline void gen_gid(void)
 {
 	SAFE_SETGID(ltp_gid);
-	printf("gid pid: %d egid: %d\n", getpid(), ltp_gid);
+	printf("gid pid: %d egid: %d rgid: %u\n", getpid(), ltp_gid, ltp_gid);
 }
 
 /*
@@ -210,8 +196,6 @@ int main(int argc, char **argv)
 	ltp_uid = ent->pw_uid;
 	ltp_gid = ent->pw_gid;
 
-	signal(SIGCHLD, SIG_IGN);
-
 	/* special processing for gen_exec, see comments above gen_exec() */
 	if (gen_event == gen_exec) {
 		exec_argv = argv;
@@ -223,8 +207,26 @@ int main(int argc, char **argv)
 	}
 
 	/* other events */
-	for (i = 0; i < nr_event; i++)
-		gen_event();
+	for (i = 0; i < nr_event; i++) {
+		pid_t pid;
+		int status;
+
+		pid = SAFE_FORK();
+		if (pid == 0) {
+			gen_event();
+			exit(0);
+		} else {
+			if (pid != SAFE_WAITPID(pid, &status, 0)) {
+				fprintf(stderr,
+				        "Child process did not terminate as expected\n");
+				return 1;
+			}
+			if (WEXITSTATUS(status) != 0) {
+				fprintf(stderr, "Child process did not terminate with 0\n");
+				return 1;
+			}
+		}
+	}
 
 	return 0;
 }
-- 
2.25.1

[LTP] [PATCH v2] pec: Fix multiple event test

[Petr Vorel]

Hi Joerg,

...
> +# Find a free file handle
> +free_fd()
> +{
> +	local fd
> +
> +	for fd in $(seq 200); do
> +		# Sapwn a new sh, because redirecting to a non existing file handle
> +		# will trigger a syntax error.
> +		/bin/sh -c ": 2>/dev/null >&$fd || : 2>/dev/null <&$fd" 2>/dev/null
Probably better to use sh -c "..."

> +		if [ $? -eq 2 ]; then
> +			echo $fd
> +			return
> +		fi
> +	done

maybe I do something wrong, but this version fails for me
(and I'm still testing it only on bash):

cn_pec 1 TINFO: timeout per run is 0h 5m 0s
cn_pec 1 TINFO: Test process events connector
cn_pec 1 TINFO: Testing fork event (nevents=10)
cn_pec 1 TBROK: No free filehandle found

I guess there is something wrong free_fd().

Kind regards,
Petr

[LTP] [PATCH v2] pec: Fix multiple event test

[Joerg Vehlow]

Hi,

On 4/16/2021 7:38 PM, Petr Vorel wrote:
> Hi Joerg,
>
> ...
>> +# Find a free file handle
>> +free_fd()
>> +{
>> +	local fd
>> +
>> +	for fd in $(seq 200); do
>> +		# Sapwn a new sh, because redirecting to a non existing file handle
>> +		# will trigger a syntax error.
>> +		/bin/sh -c ": 2>/dev/null >&$fd || : 2>/dev/null <&$fd" 2>/dev/null
> Probably better to use sh -c "..."
I used? /bin/sh, because that is the same interpreter specified in the 
shebang. Just using sh could theoretically fail or spawn a different shell.
Maybe $SHELL would be better, but I don't know how widely this is 
supported...

>
>> +		if [ $? -eq 2 ]; then
>> +			echo $fd
>> +			return
>> +		fi
>> +	done
> maybe I do something wrong, but this version fails for me
> (and I'm still testing it only on bash):
>
> cn_pec 1 TINFO: timeout per run is 0h 5m 0s
> cn_pec 1 TINFO: Test process events connector
> cn_pec 1 TINFO: Testing fork event (nevents=10)
> cn_pec 1 TBROK: No free filehandle found
>
> I guess there is something wrong free_fd().

Damn shells... I just realized /bin/sh on my system was dash's sh 
emulation and it does not emulate sh the same way bash does...
The return value in bash is 1, not two... Can you please check the patch 
with [ $? -ne 0 ]? That should work for all shells.

J?rg

[LTP] [PATCH v2] pec: Fix multiple event test

[Petr Vorel]

Hi Joerg,

> Hi,

> On 4/16/2021 7:38 PM, Petr Vorel wrote:
> > Hi Joerg,

> > ...
> > > +# Find a free file handle
> > > +free_fd()
> > > +{
> > > +	local fd
> > > +
> > > +	for fd in $(seq 200); do
> > > +		# Sapwn a new sh, because redirecting to a non existing file handle
> > > +		# will trigger a syntax error.
> > > +		/bin/sh -c ": 2>/dev/null >&$fd || : 2>/dev/null <&$fd" 2>/dev/null
> > Probably better to use sh -c "..."
> I used? /bin/sh, because that is the same interpreter specified in the
> shebang. Just using sh could theoretically fail or spawn a different shell.
> Maybe $SHELL would be better, but I don't know how widely this is
> supported...
I meant that 'sh' is on any linux system /bin/sh (or a symlink to
/usr/bin/sh, i.e. a default shell. No need to specify it with full path.
We happily use elsewhere just sh, not /bin/sh.


> > > +		if [ $? -eq 2 ]; then
> > > +			echo $fd
> > > +			return
> > > +		fi
> > > +	done
> > maybe I do something wrong, but this version fails for me
> > (and I'm still testing it only on bash):

> > cn_pec 1 TINFO: timeout per run is 0h 5m 0s
> > cn_pec 1 TINFO: Test process events connector
> > cn_pec 1 TINFO: Testing fork event (nevents=10)
> > cn_pec 1 TBROK: No free filehandle found

> > I guess there is something wrong free_fd().

> Damn shells... I just realized /bin/sh on my system was dash's sh emulation
> and it does not emulate sh the same way bash does...
Yes, unfortunately it's required to test shell tests at least on bash and dash.

I also try to test on busybox sh (ash implementation), which commonly used on
embedded (it mostly works if dash works). Some time ago I tested even on android
shell, but I give up (expecting anybody playing with LTP on android should
install busybox).

> The return value in bash is 1, not two... Can you please check the patch
> with [ $? -ne 0 ]? That should work for all shells.
Yes it works. I'll have a look at the rest of the patch today.

Kind regards,
Petr

> J?rg

[LTP] [PATCH v2] pec: Fix multiple event test

[Petr Vorel]

Hi Joerg,

thanks a lot, merged!

Kind regards,
Petr