[PATCH v4 3/4] Implement -e status=set option
Dmitry V. Levin
ldv at altlinux.org
Mon Jun 17 23:50:27 UTC 2019
On Sat, Jun 15, 2019 at 07:32:45PM +0200, Paul Chaignon wrote:
> The status qualifier enables filtering based on the return status of
> syscalls. -z and -Z become aliases for -e status=successful and -e
> status=failed. Staged output is only enabled when at least one status is
> filtered, that is, when the set is incomplete.
>
> Signed-off-by: Paul Chaignon <paul.chaignon at gmail.com>
> ---
> defs.h | 2 --
> filter_qualify.c | 33 +++++++++++++++++++++++++++++++++
> number_set.c | 32 ++++++++++++++++++++++++++++++++
> number_set.h | 13 +++++++++++++
> strace.1.in | 35 +++++++++++++++++++++++++++++++++++
> strace.c | 31 +++++++++++++++++++------------
> syscall.c | 23 +++++++++++++----------
> 7 files changed, 145 insertions(+), 24 deletions(-)
Looks good, thanks.
There are a few minor issues that should be addressed, see below.
[...]
> @@ -421,6 +453,7 @@ static const struct qual_options {
> { "x", qualify_raw },
> { "signal", qualify_signals },
> { "signals", qualify_signals },
> + { "status", qualify_status },
> { "s", qualify_signals },
> { "read", qualify_read },
> { "reads", qualify_read },
Please use the same indentation as in neighbour lines.
> diff --git a/number_set.c b/number_set.c
> index 4092ffda..878fad48 100644
> --- a/number_set.c
> +++ b/number_set.c
> @@ -47,6 +47,31 @@ reallocate_number_set(struct number_set *const set, const unsigned int new_nslot
> set->nslots = new_nslots;
> }
>
> +static unsigned int
> +popcount(const unsigned int x)
> +{
> + unsigned int count = 0;
> +
> +#ifdef HAVE___BUILTIN_POPCOUNT
> + count = __builtin_popcount(x);
> +#else
> + for (; x; ++count)
> + x &= x - 1;
> +#endif
> + return count;
> +}
We already have something similar in signal.c,
please avoid code duplication if possible.
> +static unsigned int
> +get_number_setbit(const struct number_set *const set)
> +{
> + unsigned int i;
> + unsigned int count = 0;
> +
> + for (i = 0; i < set->nslots; ++i)
> + count += popcount(set->vec[i]);
> + return count;
> +}
If we could assume sizeof(number_slot_t) == sizeof(uint32_t),
this would be equivalent to popcount32(set->vec, set->nslots).
[...]
> @@ -39,8 +42,18 @@ alloc_number_set_array(unsigned int nmemb) ATTRIBUTE_MALLOC;
> extern void
> free_number_set_array(struct number_set *, unsigned int nmemb);
>
> +enum status_t {
> + STATUS_SUCCESSFUL,
> + STATUS_FAILED,
> + STATUS_UNFINISHED,
> + STATUS_UNAVAILABLE,
> + STATUS_DETACHED,
> + __MAX_STATUS,
> +};
Identifiers that begin with an underscore are reserved,
please think of a different name.
[...]
> --- a/strace.1.in
> +++ b/strace.1.in
> @@ -408,6 +408,7 @@ is one of
> .BR read ,
> .BR write ,
> .BR fault ,
> +.BR status ,
> .BR inject ,
As "fault" is a special case of "inject", let's insert the new keyword
somewhere else.
> or
> .B kvm
> @@ -613,6 +614,40 @@ Note that this is independent from the normal tracing of the
> system call which is controlled by the option
> .BR -e "\ " trace = write .
> .TP
> +\fB\-e\ status\fR=\,\fIset\fR
> +Trace only system calls with the specified return status. The default is
> +.BR status = all .
> +When using the
> +.B status qualifier, because
> +.B strace waits for system calls to return before deciding if they should be
> +printed, the order of events may not be preserved anymore. For example, if a
> +first system call is being executed and another is called by a different
> +thread,
> +.B strace will print the first system call after the second. The first system
> +call will still be marked as
> +.IR unfinished and
> +.IR resumed , to signal to the user that it was interrupted.
> +.TP
> +.BR "\-e\ status" = successful
> +Trace system calls that returned without an error code. The
> +.B -z option has the effect of
> +.BR status=successful .
> +.TP
> +.BR "\-e\ status" = failed
> +Trace system calls that returned with an error code. The
> +.B -Z option has the effect of
> +.BR status=failed .
> +.TP
> +.BR "\-e\ status" = unfinished
> +Trace system calls that did not return. This might happen, for example, due to
> +an execve call in a neighbour thread.
exit/exit_group are also unfinished system calls, maybe it worth
mentioning them as well.
> +.TP
> +.BR "\-e\ status" = unavailable
> +Trace system calls that returned but strace failed to fetch the error status.
> +.TP
> +.BR "\-e\ status" = detached
> +Trace system calls for which strace detached before the return.
> +.TP
> \fB\-e\ inject\fR=\,\fIset\/\fR[:\fBerror\fR=\,\fIerrno\/\fR|:\fBretval\fR=\,\fIvalue\/\fR][:\fBsignal\fR=\,\fIsig\/\fR][:\fBsyscall\fR=\fIsyscall\fR][:\fBdelay_enter\fR=\,\fIusecs\/\fR][:\fBdelay_exit\fR=\,\fIusecs\/\fR][:\fBwhen\fR=\,\fIexpr\/\fR]
> Perform syscall tampering for the specified set of syscalls.
Unfortunately, due to unusual formatting the whole description
of status keyword looks ugly.
> diff --git a/strace.c b/strace.c
> index 6c3d68c0..baaa771c 100644
> --- a/strace.c
> +++ b/strace.c
> @@ -109,10 +109,6 @@ static bool daemonized_tracer;
> static int post_attach_sigstop = TCB_IGNORE_ONE_SIGSTOP;
> #define use_seize (post_attach_sigstop == 0)
>
> -/* Sometimes we want to print succeeding/failing syscalls only. */
> -bool not_failing_only;
> -bool failing_only;
> -
> /* Show path associated with fd arguments */
> unsigned int show_fd_path;
>
> @@ -273,7 +269,7 @@ Statistics:\n\
> \n\
> Filtering:\n\
> -e expr a qualifying expression: option=[!]all or option=[!]val1[,val2]...\n\
> - options: trace, abbrev, verbose, raw, signal, read, write, fault, inject, kvm\n\
> + options: trace, abbrev, verbose, raw, signal, read, write, fault, status, inject, kvm\n\
Likewise, since "fault" is a special case of "inject", let's insert
the new keyword somewhere else in the line.
[...]
> @@ -760,13 +765,6 @@ syscall_exiting_trace(struct tcb *tcp, struct timespec *ts, int res)
> if (raw(tcp)) {
> /* sys_res = printargs(tcp); - but it's nop on sysexit */
> } else {
> - if ((not_failing_only && syserror(tcp)) ||
> - (failing_only && !syserror(tcp))) {
> - strace_close_memstream(tcp, false);
> - line_ended();
> - return 0; /* ignore failed/successful
> - * syscalls */
> - }
> if (tcp->sys_func_rval & RVAL_DECODED)
> sys_res = tcp->sys_func_rval;
> else
Interesting. Is there any particular reason to remove this optimization?
--
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20190618/b6998dd1/attachment.bin>
More information about the Strace-devel
mailing list