[PATCHv1] strace: add :signal option to efault

Dmitry V. Levin ldv at altlinux.org
Mon Dec 26 22:59:15 UTC 2016


On Mon, Dec 26, 2016 at 06:24:46PM +0100, Seraphime Kirkovski wrote:
> This extends the -e fault capability with a :signal option which
> delivers a signal on entry of the specified syscall.
> 
> :signal and :error are complementary, if they are both specified the
> syscall will be injected as per normal and a signal will be sent to the
> process.
> 
> Signed-off-by: Seraphime Kirkovski <kirkseraph at gmail.com>
> ---
> This takes account of the changes requested by Dmitry, i.e.
> this makes :signal and :error complementary rather than mutually exclusive.

Nice work.  See my comments below.

>  defs.h    |  4 +++-
>  qualify.c | 11 ++++++++++-
>  strace.c  |  4 +++-
>  syscall.c | 16 +++++++++-------
>  4 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/defs.h b/defs.h
> index 0a4c616..597bdd3 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -218,6 +218,7 @@ struct fault_opts {
>  	uint16_t first;
>  	uint16_t step;
>  	uint16_t err;
> +	int16_t signo;

Since signal 0 means no signal, do we need it to be signed?  
Conversely, as your change essentially makes err signed,
wouldn't it be better to change its type to int16_t?

>  };
>  
>  #if defined LINUX_MIPSN32 || defined X32
> @@ -457,13 +458,14 @@ extern int read_int_from_file(const char *, int *);
>  extern void set_sortby(const char *);
>  extern void set_overhead(int);
>  extern void print_pc(struct tcb *);
> -extern int trace_syscall(struct tcb *);
> +extern int trace_syscall(struct tcb *, unsigned int *);
>  extern void count_syscall(struct tcb *, const struct timeval *);
>  extern void call_summary(FILE *);
>  
>  extern void clear_regs(void);
>  extern void get_regs(pid_t pid);
>  extern int get_scno(struct tcb *tcp);
> +

No need to add an empty line along with this change.

>  /**
>   * Convert syscall number to syscall name.
>   *
> diff --git a/qualify.c b/qualify.c
> index a7d276c..d707bfe 100644
> --- a/qualify.c
> +++ b/qualify.c
> @@ -414,6 +414,11 @@ parse_fault_token(const char *const token, struct fault_opts *const fopts)
>  		if (intval < 1)
>  			return false;
>  		fopts->err = intval;
> +	} else if ((val = strip_prefix("signal=", token))) {
> +		intval = sigstr_to_uint(val);
> +		if (intval < 0)
> +			return false;

Since signal 0 means no signal, the check should rather be for (intval < 1).

> +		fopts->signo = intval;
>  	} else {
>  		return false;
>  	}
> @@ -494,13 +499,17 @@ qualify_fault(const char *const str)
>  	struct fault_opts opts = {
>  		.first = 1,
>  		.step = 1,
> -		.err = 0
> +		.signo = -1,
> +		.err = -1

Please keep the former order of initialization.
Since signal 0 means no signal, we could initialize .signo with 0 as well.

>  	};
>  	char *buf = NULL;
>  	char *name = parse_fault_expression(str, &buf, &opts);
>  	if (!name) {
>  		error_msg_and_die("invalid %s '%s'", "fault argument", str);
>  	}
> +	/* If no error, nor signal is specified, fallback to ENOSYS */

neither error nor signal

> +	if (opts.signo == -1 && (int16_t)opts.err == -1)

Please add a space after cast.
Assuming that opts.err has type int16_t and signal 0 means no signal,
the check could be written as

	if (opts.signo == 0 && opts.err == -1)

> +		opts.err = ENOSYS;

This would change behaviour on systems like s390 where the default
is EPERM instead of ENOSYS.  I think we can force ENOSYS upon s390,
though.

>  	struct number_set tmp_set[SUPPORTED_PERSONALITIES];
> diff --git a/strace.c b/strace.c
> index 4659ddb..23ca108 100644
> --- a/strace.c
> +++ b/strace.c
> @@ -2447,7 +2447,8 @@ show_stopsig:
>  	 * This should be syscall entry or exit.
>  	 * Handle it.
>  	 */
> -	if (trace_syscall(tcp) < 0) {
> +	sig = 0;
> +	if (trace_syscall(tcp, &sig) < 0) {
>  		/*
>  		 * ptrace() failed in trace_syscall().
>  		 * Likely a result of process disappearing mid-flight.
> @@ -2461,6 +2462,7 @@ show_stopsig:
>  		 */
>  		return true;
>  	}
> +	goto restart_tracee;
>  
>  restart_tracee_with_sig_0:
>  	sig = 0;
> diff --git a/syscall.c b/syscall.c
> index 63d3b00..e4fd36b 100644
> --- a/syscall.c
> +++ b/syscall.c
> @@ -573,7 +573,7 @@ tcb_fault_opts(struct tcb *tcp)
>  
>  
>  static long
> -inject_syscall_fault_entering(struct tcb *tcp)
> +inject_syscall_fault_entering(struct tcb *tcp, unsigned int *signo)
>  {
>  	if (!tcp->fault_vec[current_personality]) {
>  		tcp->fault_vec[current_personality] =
> @@ -595,7 +595,9 @@ inject_syscall_fault_entering(struct tcb *tcp)
>  
>  	opts->first = opts->step;
>  
> -	if (!arch_set_scno(tcp, -1))
> +	if (opts->signo != -1)
> +		*signo = opts->signo;

If we keep to the idiom that signal 0 means no signal, then the check
could be written as

	if (opts->signo != 0)

> +	if ((int16_t)opts->err != -1 && !arch_set_scno(tcp, -1))

Please add a space after cast, or, alternatively,
change opts->err type to int16_t and drop the cast.

>  		tcp->flags |= TCB_FAULT_INJ;
>  
>  	return 0;
> @@ -617,7 +619,7 @@ update_syscall_fault_exiting(struct tcb *tcp)
>  }
>  
>  static int
> -trace_syscall_entering(struct tcb *tcp)
> +trace_syscall_entering(struct tcb *tcp, unsigned int *sig)
>  {
>  	int res, scno_good;
>  
> @@ -688,7 +690,7 @@ trace_syscall_entering(struct tcb *tcp)
>  	}
>  
>  	if (tcp->qual_flg & QUAL_FAULT)
> -		inject_syscall_fault_entering(tcp);
> +		inject_syscall_fault_entering(tcp, sig);
>  
>  	if (cflag == CFLAG_ONLY_STATS) {
>  		res = 0;
> @@ -716,7 +718,7 @@ trace_syscall_entering(struct tcb *tcp)
>  	/* Measure the entrance time as late as possible to avoid errors. */
>  	if (Tflag || cflag)
>  		gettimeofday(&tcp->etime, NULL);
> -	return res;
> +	return 0;

Why do you need to ignore return code?
This looks wrong and will surely break things.

>  }
>  
>  static bool
> @@ -981,10 +983,10 @@ trace_syscall_exiting(struct tcb *tcp)
>  }
>  
>  int
> -trace_syscall(struct tcb *tcp)
> +trace_syscall(struct tcb *tcp, unsigned int *signo)
>  {
>  	return exiting(tcp) ?
> -		trace_syscall_exiting(tcp) : trace_syscall_entering(tcp);
> +		trace_syscall_exiting(tcp) : trace_syscall_entering(tcp, signo);
>  }
>  
>  bool

Please update the description of fault=SET syntax in strace(1) manual page.

Do you think you could also write a test for this new feature?


-- 
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/20161227/c0775fd7/attachment.bin>


More information about the Strace-devel mailing list