[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