[PATCH 4/4] tests: check syscall fault injection behavior and error handling

[Dmitry V. Levin]

On Wed, Jul 27, 2016 at 08:41:17PM +0200, Nahim El Atmani wrote:
> +# include <signal.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <string.h>
> +# include <unistd.h>

What a strange indentation.

> +static void
> +handler(int sig)
> +{
> +}

This handler doesn't do much, why not use SIG_IGN?

> +static void
> +occ_at(int raw)
> +{
> +	pid_t pid = getpid();
> +	syscall(__NR_kill, pid, SIGALRM);
> +	syscall(__NR_kill, pid, SIGALRM);
> +	if (!raw)
> +		printf("kill(%d, SIGALRM) = 0\n"
> +		       "kill(%d, SIGALRM) = -1 EINVAL (Invalid argument)(DISCARDED)\n"

This "(DISCARDED)" doesn't look quite informative.  If this syscall failure
is due to fault injection, why not write something to the point, e.g.
" (fault injected)"?

> +		       "+++ exited with 0 +++\n", pid, pid);
> +	else
> +		printf("kill(%d, 0xe) = 0\n"
> +		       "kill(%d, 0xe) = -1 (errno 22)(DISCARDED)\n"
> +		       "+++ exited with 0 +++\n", pid, pid);
> +}

This alteration in expected output doesn't help to detect the case when
"at" mode is implemented as "every".

You don't have to hardcode values of  SIGALRM and EINVAL.

> +static void
> +occ_every(void)
> +{
> +	pid_t pid = getpid();
> +	syscall(__NR_kill, pid, SIGALRM);
> +	syscall(__NR_kill, pid, SIGALRM);
> +	syscall(__NR_kill, pid, SIGALRM);
> +	syscall(__NR_kill, pid, SIGALRM);
> +	syscall(__NR_kill, pid, SIGALRM);
> +	syscall(__NR_kill, pid, SIGALRM);
> +	printf("kill(%d, SIGALRM) = 0\n"
> +	       "kill(%d, SIGALRM) = -1 EINVAL (Invalid argument)(DISCARDED)\n"
> +	       "kill(%d, SIGALRM) = 0\n"
> +	       "kill(%d, SIGALRM) = -1 EINVAL (Invalid argument)(DISCARDED)\n"
> +	       "kill(%d, SIGALRM) = 0\n"
> +	       "kill(%d, SIGALRM) = -1 EINVAL (Invalid argument)(DISCARDED)\n"
> +	       "+++ exited with 0 +++\n", pid, pid, pid, pid, pid, pid);
> +}

Why not just send a fixed batch of signals regardless of testing mode
and print mode specific expected output?

> +# Ensure that strace -e faultwith=<syscall>:<occurrence>:<error>,... works.
> +
> +. "${srcdir=.}/init.sh"
> +
> +run_strace -a0 -e trace=kill -e fault=kill:2:EINVAL -e signal=none ./fault at > "$EXP"
> +match_diff "$EXP" "$LOG"
> +run_strace -a0 -e trace=kill -e fault=kill:2.:EINVAL -e signal=none ./fault every > "$EXP"
> +match_diff "$EXP" "$LOG"
> +run_strace -a0 -e trace=kill -e raw=kill -e fault=kill:2.:EINVAL -e signal=none ./fault raw > "$EXP"
> diff --git a/tests/fault_parsing.test b/tests/fault_parsing.test

What's the purpose of the last run_strace?

You could avoid this mistake by writing a common test function
and calling it thrice with different parameters.

> +# Ensure that strace -e faultwith=<syscall>:<occurrence>:<error>,... works.
> +
> +. "${srcdir=.}/init.sh"
> +
> +INV_SC_NAME="invalid_syscall_name"
> +INV_ER_NAME="invalid_error_name"
> +INV_OCC_NBR1="-1"
> +INV_OCC_NBR2="105%"
> +ERR_MSG='invalid fault argument'
> +$(./is_x86)
> +X86=$?
> +
> +if [ ! $X86 ] # Parser check
> +then
> +    skip_ "fault parsing not available for non x86 architecture"
> +else
> +    if [ $X86 == 1 ] # 32 bit
> +    then
> +	run_strace -a0 -e trace=kill -e fault=37:2:22 -e signal=none ./fault at > "$EXP"
> +	match_diff "$EXP" "$LOG"
> +    elif [ $X86 == 2 ] # 64 bit
> +    then
> +	run_strace -a0 -e trace=kill -e fault=62:2:22 -e signal=none ./fault at > "$EXP"
> +	match_diff "$EXP" "$LOG"
> +    fi
> +    rm $EXP $LOG

This hardcoding of syscall numbers is awful and it doesn't scale anyway.
Why can't the helper just return __NR_kill?

> +    $STRACE -a0 -e trace=kill -e fault="$INV_SC_NAME":2:EINVAL ./fault at 2> "$LOG"
> +    grep -v "$ERR_MSG" < "$LOG" && dump_log_and_fail_with 'strace-fault failed'
> +    $STRACE -a0 -e trace=kill -e fault=kill:2:"$INV_ER_NAME" ./fault at 2> "$LOG"
> +    grep -v "$ERR_MSG" < "$LOG" && dump_log_and_fail_with 'strace-fault failed'
> +    $STRACE -a0 -e trace=kill -e fault=kill:"$INV_OCC_NBR1":EAGAIN ./fault at 2> "$LOG"
> +    grep -v "$ERR_MSG" < "$LOG" && dump_log_and_fail_with 'strace-fault failed'
> +    $STRACE -a0 -e trace=kill -e fault=kill:"$INV_OCC_NBR2":EAGAIN ./fault at 2> "$LOG"
> +    grep -v "$ERR_MSG" < "$LOG" && dump_log_and_fail_with 'strace-fault failed'

A common test function would be much easier to read.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20160729/0109b3bd/attachment.bin>