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 <kirkser...@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
pgp92qa5cUlAf.pgp
Description: PGP signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
