[PATCH v5 3/4] Implement -e status=set option
Dmitry V. Levin
ldv at altlinux.org
Mon Jul 1 23:26:44 UTC 2019
On Fri, Jun 28, 2019 at 11:34:21AM +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.
>
> * defs.h (not_failing_only): Remove variable.
> (failing_only): Remove variable.
> (popcount32): Move function from signal.c.
> * signal.c (popcount32): Move function to defs.h.
The preferred way of writing this is:
* signal.c (popcount32): Move ...
* defs.h: ... here.
(not_failing_only, failing_only): Remove.
> * filter_qualify.c (status_set): New number_set variable.
> (statuses): New variable for names of statuses.
> (statusstr_to_uint): New function.
> (qualify_status): New function.
(statusstr_to_uint, qualify_status): New functions.
> (qual_options): Handle status qualifier.
> * number_set.c (get_number_setbit): New function.
> (is_complete_set): New function.
> * number_set.h (is_complete_set): New prototype.
> (status_t): New enumeration for statuses.
> (status_set): Expose status_set variable.
> * strace.1.in: Document new status qualifier.
> * strace.c (not_failing_only): Remove variable.
> (failing_only): Remove variable.
> (droptcb): Handle status=detached option.
> (init): Handle new status qualifier, set status_set variable on -z and -Z
> options, warning on -zZ, use is_complete_set.
> (maybe_switch_tcbs): Reopen memstream after tcb switch.
> (print_event_exit): Handle status=unfinished option.
> * syscall.c (syscall_entering_trace): Use is_complete_set.
> (syscall_exiting_trace): Use is_complete_set, handle status=unavailable
> option.
The patch looks almost ready, please see my comments below.
> ---
> defs.h | 24 ++++++++++++++++++++++--
> filter_qualify.c | 33 +++++++++++++++++++++++++++++++++
> number_set.c | 17 +++++++++++++++++
> number_set.h | 13 +++++++++++++
> signal.c | 19 -------------------
> strace.1.in | 41 +++++++++++++++++++++++++++++++++++++++++
> strace.c | 45 +++++++++++++++++++++++++++++++++------------
> syscall.c | 25 ++++++++++++++++---------
> 8 files changed, 175 insertions(+), 42 deletions(-)
>
> diff --git a/defs.h b/defs.h
> index b9b394ec..b6140fe0 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -420,8 +420,6 @@ extern bool Tflag;
> extern bool iflag;
> extern bool count_wallclock;
> extern unsigned int qflag;
> -extern bool not_failing_only;
> -extern bool failing_only;
> extern unsigned int show_fd_path;
> /* are we filtering traces based on paths? */
> extern struct path_set {
> @@ -1451,6 +1449,28 @@ truncate_kulong_to_current_wordsize(const kernel_ulong_t v)
> sizeof(v) == sizeof(long) ? (long long) (long) (v) : \
> (long long) (v))
>
> +/*
> + * Computes the popcount of a vector of 32-bit values.
> + */
> +static inline unsigned int
> +popcount32(const uint32_t *a, unsigned int size)
> +{
> + unsigned int count = 0;
> +
> + for (; size; ++a, --size) {
> + uint32_t x = *a;
> +
> +#ifdef HAVE___BUILTIN_POPCOUNT
> + count += __builtin_popcount(x);
> +#else
> + for (; x; ++count)
> + x &= x - 1;
> +#endif
> + }
> +
> + return count;
> +}
> +
> extern const char *const errnoent[];
> extern const char *const signalent[];
> extern const unsigned int nerrnos;
> diff --git a/filter_qualify.c b/filter_qualify.c
> index 4a05f1b2..6e7c5ef2 100644
> --- a/filter_qualify.c
> +++ b/filter_qualify.c
> @@ -12,10 +12,12 @@
> #include "filter.h"
> #include "delay.h"
> #include "retval.h"
> +#include "static_assert.h"
>
> struct number_set *read_set;
> struct number_set *write_set;
> struct number_set *signal_set;
> +struct number_set *status_set;
>
> static struct number_set *abbrev_set;
> static struct number_set *inject_set;
> @@ -57,6 +59,28 @@ sigstr_to_uint(const char *s)
> return -1;
> }
>
> +static const char *statuses[] = {
> + "successful",
> + "failed",
> + "unfinished",
> + "unavailable",
> + "detached",
> +};
> +static_assert(sizeof(statuses) / sizeof(char *) == NUMBER_OF_STATUSES,
ARRAY_SIZE(statuses)?
> + "statuses array and status_t enum mismatch");
> +
> +static int
> +statusstr_to_uint(const char *str)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < NUMBER_OF_STATUSES; ++i)
> + if (strcasecmp(str, statuses[i]) == 0)
> + return i;
> +
> + return -1;
> +}
> +
> static int
> find_errno_by_name(const char *name)
> {
> @@ -275,6 +299,14 @@ qualify_signals(const char *const str)
> qualify_tokens(str, signal_set, sigstr_to_uint, "signal");
> }
>
> +static void
> +qualify_status(const char *const str)
> +{
> + if (!status_set)
> + status_set = alloc_number_set_array(1);
> + qualify_tokens(str, status_set, statusstr_to_uint, "status");
> +}
> +
> static void
> qualify_trace(const char *const str)
> {
> @@ -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 },
> diff --git a/number_set.c b/number_set.c
> index 4092ffda..70727eee 100644
> --- a/number_set.c
> +++ b/number_set.c
> @@ -12,7 +12,9 @@
> #include <stdbool.h>
> #include <stdlib.h>
> #include <string.h>
> +#include "defs.h"
> #include "number_set.h"
> +#include "static_assert.h"
> #include "xmalloc.h"
>
> typedef unsigned int number_slot_t;
> @@ -47,6 +49,14 @@ reallocate_number_set(struct number_set *const set, const unsigned int new_nslot
> set->nslots = new_nslots;
> }
>
> +static unsigned int
> +get_number_setbit(const struct number_set *const set)
> +{
> + static_assert(sizeof(number_slot_t) == sizeof(uint32_t),
> + "number_slot_t is not 32-bit long");
> + return popcount32(set->vec, set->nslots);
> +}
> +
> bool
> number_set_array_is_empty(const struct number_set *const set,
> const unsigned int idx)
> @@ -69,6 +79,13 @@ is_number_in_set_array(const unsigned int number, const struct number_set *const
> && number_isset(number, set[idx].vec)) ^ set[idx].not;
> }
>
> +bool
> +is_complete_set(const struct number_set *const set, const unsigned int max_numbers)
> +{
> + return set && ((set->not && !set->nslots) ||
> + (get_number_setbit(set) == max_numbers));
> +}
> +
> void
> add_number_to_set(const unsigned int number, struct number_set *const set)
> {
> diff --git a/number_set.h b/number_set.h
> index 77dc3a9c..b6399cc8 100644
> --- a/number_set.h
> +++ b/number_set.h
> @@ -21,6 +21,9 @@ is_number_in_set(unsigned int number, const struct number_set *);
> extern bool
> is_number_in_set_array(unsigned int number, const struct number_set *, unsigned int idx);
>
> +extern bool
> +is_complete_set(const struct number_set *, unsigned int max_numbers);
> +
> extern void
> add_number_to_set(unsigned int number, struct number_set *);
>
> @@ -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,
> + NUMBER_OF_STATUSES
> +};
> +
> extern struct number_set *read_set;
> extern struct number_set *write_set;
> extern struct number_set *signal_set;
> +extern struct number_set *status_set;
>
> #endif /* !STRACE_NUMBER_SET_H */
> diff --git a/signal.c b/signal.c
> index fcaf9d4f..3cb54bb3 100644
> --- a/signal.c
> +++ b/signal.c
> @@ -138,25 +138,6 @@ sprintsigname(const int sig)
> return buf;
> }
>
> -static unsigned int
> -popcount32(const uint32_t *a, unsigned int size)
> -{
> - unsigned int count = 0;
> -
> - for (; size; ++a, --size) {
> - uint32_t x = *a;
> -
> -#ifdef HAVE___BUILTIN_POPCOUNT
> - count += __builtin_popcount(x);
> -#else
> - for (; x; ++count)
> - x &= x - 1;
> -#endif
> - }
> -
> - return count;
> -}
> -
> const char *
> sprintsigmask_n(const char *prefix, const void *sig_mask, unsigned int bytes)
> {
> diff --git a/strace.1.in b/strace.1.in
> index e1090a0f..da31af6b 100644
> --- a/strace.1.in
> +++ b/strace.1.in
> @@ -409,6 +409,7 @@ is one of
> .BR write ,
> .BR fault ,
> .BR inject ,
> +.BR status ,
> or
> .B kvm
> and
> @@ -613,6 +614,46 @@ 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
Do you mean that both entering and exiting of another syscall happen after
entering but before exiting of the first syscall?
Please reword this example to make this reordering case clear.
> +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.
The most common case for unfinished syscalls is exit/exit_group.
I think it worth mentioning.
> +.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.
>
> diff --git a/strace.c b/strace.c
> index cb1d1840..2f956253 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, inject, status, kvm\n\
> -P path trace accesses to path\n\
> -z print only syscalls that returned without an error code\n\
> -Z print only syscalls that returned with an error code\n\
> @@ -812,12 +808,18 @@ droptcb(struct tcb *tcp)
> debug_msg("dropped tcb for pid %d, %d remain", tcp->pid, nprocs);
>
> if (tcp->outf) {
> + bool publish = true;
> + if (!is_complete_set(status_set, NUMBER_OF_STATUSES)) {
> + publish = is_number_in_set(STATUS_DETACHED, status_set);
> + strace_close_memstream(tcp, publish);
> + }
> +
> if (followfork >= 2) {
> - if (tcp->curcol != 0)
> + if (tcp->curcol != 0 && publish)
> fprintf(tcp->outf, " <detached ...>\n");
> fclose(tcp->outf);
> } else {
> - if (printing_tcp == tcp && tcp->curcol != 0)
> + if (printing_tcp == tcp && tcp->curcol != 0 && publish)
> fprintf(tcp->outf, " <detached ...>\n");
> flush_tcp_output(tcp);
> }
> @@ -1553,7 +1555,7 @@ static void ATTRIBUTE_NOINLINE
> init(int argc, char *argv[])
> {
> int c, i;
> - int optF = 0;
> + int optF = 0, zflags = 0;
>
> if (!program_invocation_name || !*program_invocation_name) {
> static char name[] = "strace";
> @@ -1571,6 +1573,7 @@ init(int argc, char *argv[])
> qualify("trace=all");
> qualify("abbrev=all");
> qualify("verbose=all");
> + qualify("status=all");
> #if DEFAULT_QUAL_FLAGS != (QUAL_TRACE | QUAL_ABBREV | QUAL_VERBOSE)
> # error Bug in DEFAULT_QUAL_FLAGS
> #endif
> @@ -1708,10 +1711,14 @@ init(int argc, char *argv[])
> show_fd_path++;
> break;
> case 'z':
> - not_failing_only = 1;
> + clear_number_set_array(status_set, 1);
> + add_number_to_set(STATUS_SUCCESSFUL, status_set);
> + zflags++;
> break;
> case 'Z':
> - failing_only = 1;
> + clear_number_set_array(status_set, 1);
> + add_number_to_set(STATUS_FAILED, status_set);
> + zflags++;
> break;
> default:
> error_msg_and_help(NULL);
> @@ -1764,10 +1771,14 @@ init(int argc, char *argv[])
> }
>
> #ifndef HAVE_OPEN_MEMSTREAM
> - if (not_failing_only || failing_only)
> - error_msg_and_help("open_memstream is required to use -z or -Z");
> + if (!is_complete_set(status_set, NUMBER_OF_STATUSES))
> + error_msg_and_help("open_memstream is required to use -z, -Z, or -e status");
> #endif
>
> + if (zflags > 1)
> + error_msg("Only the second flag of -z/-Z will take effect. "
> + "See status qualifier for more complex filters.");
I's say that only the last of -z/-Z options will take effect.
> +
> acolumn_spaces = xmalloc(acolumn + 1);
> memset(acolumn_spaces, ' ', acolumn);
> acolumn_spaces[acolumn] = '\0';
> @@ -2093,6 +2104,12 @@ maybe_switch_tcbs(struct tcb *tcp, const int pid)
> printleader(tcp);
> tprintf("+++ superseded by execve in pid %lu +++\n", old_pid);
> line_ended();
> + /*
> + * Need to reopen memstream for thread
> + * as we closed it in droptcb.
> + */
> + if (!is_complete_set(status_set, NUMBER_OF_STATUSES))
> + strace_open_memstream(tcp);
> tcp->flags |= TCB_REPRINT;
> }
>
> @@ -2208,6 +2225,10 @@ print_event_exit(struct tcb *tcp)
> tprints(") ");
> tabto();
> tprints("= ?\n");
> + if (!is_complete_set(status_set, NUMBER_OF_STATUSES)) {
> + bool publish = is_number_in_set(STATUS_UNFINISHED, status_set);
> + strace_close_memstream(tcp, publish);
> + }
> line_ended();
> }
>
> diff --git a/syscall.c b/syscall.c
> index 4121b7ae..d106967d 100644
> --- a/syscall.c
> +++ b/syscall.c
> @@ -656,7 +656,7 @@ syscall_entering_trace(struct tcb *tcp, unsigned int *sig)
> }
> #endif
>
> - if (not_failing_only || failing_only)
> + if (!is_complete_set(status_set, NUMBER_OF_STATUSES))
> strace_open_memstream(tcp);
>
> printleader(tcp);
> @@ -751,6 +751,11 @@ syscall_exiting_trace(struct tcb *tcp, struct timespec *ts, int res)
> tprints(") ");
> tabto();
> tprints("= ? <unavailable>\n");
> + if (!is_complete_set(status_set, NUMBER_OF_STATUSES)) {
> + bool publish = is_number_in_set(STATUS_UNAVAILABLE,
> + status_set);
> + strace_close_memstream(tcp, publish);
> + }
> line_ended();
> return res;
> }
> @@ -766,14 +771,16 @@ syscall_exiting_trace(struct tcb *tcp, struct timespec *ts, int res)
> sys_res = tcp_sysent(tcp)->sys_func(tcp);
> }
>
> - if ((not_failing_only && syserror(tcp)) ||
> - (failing_only && !syserror(tcp))) {
> - strace_close_memstream(tcp, false);
> - line_ended();
> - return 0; /* ignore failed/successful
> - * syscalls */
> - } else if (not_failing_only || failing_only) {
> - strace_close_memstream(tcp, true);
> + if (!is_complete_set(status_set, NUMBER_OF_STATUSES)) {
> + bool publish = syserror(tcp)
> + && is_number_in_set(STATUS_FAILED, status_set);
> + publish |= !syserror(tcp)
> + && is_number_in_set(STATUS_SUCCESSFUL, status_set);
> + strace_close_memstream(tcp, publish);
> + if (!publish) {
> + line_ended();
> + return 0;
> + }
> }
>
> tprints(") ");
I've got no more comments about 3/4.
--
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/20190702/ad865780/attachment.bin>
More information about the Strace-devel
mailing list