[PATCH v2 4/5] Extend -Y option to decode PIDS in arguments and retvals
Masatake YAMATO
yamato at redhat.com
Fri Sep 3 18:56:46 UTC 2021
Thank you for reviewing.
I will reflect your comments to v3 patch set.
Masatake YAMATO
On Wed, 25 Aug 2021 17:57:43 +0200, Ákos Uzonyi <uzonyi.akos at gmail.com> wrote:
> On Tue, 24 Aug 2021 at 21:27, Masatake YAMATO <yamato at redhat.com> wrote:
>> From: <yamato at redhat.com>
>>
>> From: Masatake YAMATO <yamato at redhat.com>
>>
>> This change introduces -YY,--decode-pids=all option. In addition to
>> what printed -Y option, the new options print command names for PIDS
>> appeared in arguments and retvals.
>>
>> An example:
>>
>> getppid() = 3781395<strace>
>> getpid() = 3781398<a.out>
>> pidfd_open(1<systemd>, 0) = 3<pid:1<systemd>>
>>
>> * src/defs.h (maybe_printpid_comm): New function declaration.
>> * src/strace.c (maybe_printpid_comm): New function printing
>> command name for given PID.
>> (usage): Add help message for the -YY option.
>> (init): Add -YY and --decode-pids=all options.
>> * src/pidns.c (printpid_translation,printpid,printpid_tgid_pgid): call
>> `maybe_printpid_comm'.
>> * src/filter_qualify.c (qualify_decode_pid): Handle "all" specified in
>> --decode-pids=all.
>> * src/time.c (printclockname): Print command name for pid.
>> * src/util.c (printpidfd): Ditto.
>> * doc/strace.1.in (.SS Output format): document the -YY option.
>>
>> Signed-off-by: Masatake YAMATO <yamato at redhat.com>
>> ---
>> doc/strace.1.in | 6 ++++++
>> src/defs.h | 2 ++
>> src/filter_qualify.c | 2 ++
>> src/pidns.c | 11 +++++++++--
>> src/strace.c | 24 ++++++++++++++++++++++--
>> src/time.c | 4 +++-
>> src/util.c | 1 +
>> 7 files changed, 45 insertions(+), 5 deletions(-)
>>
>> diff --git a/doc/strace.1.in b/doc/strace.1.in
>> index 99fba9076..28358f7df 100644
>> --- a/doc/strace.1.in
>> +++ b/doc/strace.1.in
>> @@ -1089,6 +1089,12 @@ and PIDs associated with pidfd file descriptors.
>> .BR \-\-decode\-pids = prefix
>> Print command names for PIDs that appear as the prefixes of trace lines.
>> .TP
>> +.B \-YY
>> +.TQ
>> +.BR \-\-decode\-pids = all
>> +Print command names for PIDs that appear in arguments and return values
>> +in addition to what printed with \-Y option.
>> +.TP
>> .B \-\-pidns\-translation
>> If strace and tracee are in different PID namespaces, print PIDs in
>> strace's namespace, too.
>> diff --git a/src/defs.h b/src/defs.h
>> index 33f44dd64..5a4d418ee 100644
>> --- a/src/defs.h
>> +++ b/src/defs.h
>> @@ -1909,4 +1909,6 @@ print_big_u64_addr(const uint64_t addr)
>> # endif
>>
>> void maybe_load_task_comm(struct tcb *tcp);
>> +/* print the content of /proc/$pid/comm. */
>> +void maybe_printpid_comm(int pid);
>> #endif /* !STRACE_DEFS_H */
>> diff --git a/src/filter_qualify.c b/src/filter_qualify.c
>> index 24e840c2f..b871176b1 100644
>> --- a/src/filter_qualify.c
>> +++ b/src/filter_qualify.c
>> @@ -468,6 +468,8 @@ qualify_decode_pid(const char *const str)
>> {
>> if (strcmp(str, "prefix") == 0)
>> decode_pids_enabled = 1;
>> + else if (strcmp(str, "all") == 0)
>> + decode_pids_enabled = 2;
>> else
>> error_msg_and_die("invalid --decode-pids = argument: '%s'", str);
>> }
>> diff --git a/src/pidns.c b/src/pidns.c
>> index d3ebd0535..57f695d22 100644
>> --- a/src/pidns.c
>> +++ b/src/pidns.c
>> @@ -586,6 +586,8 @@ printpid_translation(struct tcb *tcp, int pid, enum pid_type type)
>> return;
>>
>> int strace_pid = translate_pid(tcp, pid, type, NULL);
>> + if (strace_pid && (type == PT_TID || type == PT_TGID))
>> + maybe_printpid_comm(strace_pid);
>> if (strace_pid && strace_pid != pid)
>> tprintf_comment("%d in strace's PID NS", strace_pid);
>> }
>> @@ -594,6 +596,9 @@ void
>> printpid(struct tcb *tcp, int pid, enum pid_type type)
>> {
>> PRINT_VAL_D(pid);
>> + if (!pidns_translation
>> + && (type == PT_TID || type == PT_TGID))
>> + maybe_printpid_comm(pid);
>> printpid_translation(tcp, pid, type);
>> }
>>
>> @@ -601,8 +606,10 @@ void
>> printpid_tgid_pgid(struct tcb *tcp, int pid)
>> {
>> PRINT_VAL_D(pid);
>> - if (pid > 0)
>> + if (pid > 0) {
>> + if (!pidns_translation)
>> + maybe_printpid_comm(pid);
>> printpid_translation(tcp, pid, PT_TGID);
>> - else if (pid < -1)
>> + } else if (pid < -1)
>> printpid_translation(tcp, -pid, PT_PGID);
>> }
>
> Please note that even if pidns_translation is disabled, the process of
> the tcp can still be in a different PID namespace. I would suggest to
> modify printpid_translation, to do the translation not only when
> pidns_translation is enabled, but also when decode_pids_enabled >= 2,
> to be able to pass the correct PID (in strace's namespace) to
> maybe_printpid_comm. With these modifications it won't be necessary to
> call maybe_printpid_comm from other places than pidns_translation (and
> maybe we could rename printpid_translation to something else, as it
> also prints comm field now).
>
>> diff --git a/src/strace.c b/src/strace.c
>> index 26d11b271..da4178367 100644
>> --- a/src/strace.c
>> +++ b/src/strace.c
>> @@ -53,7 +53,9 @@ bool stack_trace_enabled;
>> #endif
>>
>> /* if this is non-zero
>> - * print the process names read from /proc/$pid/comm files */
>> + * print the process names read from /proc/$pid/comm files
>> + * 1: prefix
>> + * 2: all (prefix, arguments and return values) */
>> unsigned decode_pids_enabled;
>>
>> #define my_tkill(tid, sig) syscall(__NR_tkill, (tid), (sig))
>> @@ -421,6 +423,10 @@ Output format:\n\
>> -Y, --decode-pids[=prefix]\n\
>> print command names associated with the pids appeared\n\
>> as prefixes of trace lines\n\
>> + -YY, --decode-pids=all\n\
>> + print command names associated with the pids appeared\n\
>> + as arguments and return values in addition to what printed\n\
>> + with -Y option.\n\
>> "
>> #ifdef ENABLE_SECONTEXT
>> "\
>> @@ -946,6 +952,19 @@ load_pid_comm(int pid, char *buf, size_t buf_size)
>> fclose(fp);
>> }
>>
>> +void
>> +maybe_printpid_comm(int pid)
>> +{
>> + if (decode_pids_enabled < 2)
>> + return;
>> +
>> + char buf[PROC_COMM_LEN];
>> + load_pid_comm(pid, buf, sizeof(buf));
>> + tprints("<");
>> + tprints(buf);
>> + tprints(">");
>> +}
>> +
>> void
>> maybe_load_task_comm(struct tcb *tcp)
>> {
>> @@ -2039,6 +2058,7 @@ init(int argc, char *argv[])
>> static const char yflag_qual[] = "path";
>> static const char yyflag_qual[] = "all";
>> static const char Yflag_qual[] = "prefix";
>> + static const char YYflag_qual[] = "all";
>> static const char tflag_str[] = "format:time";
>> static const char ttflag_str[] = "precision:us,format:time";
>> static const char tttflag_str[] = "format:unix,precision:us";
>> @@ -2532,7 +2552,7 @@ init(int argc, char *argv[])
>> " be provided simultaneously");
>> }
>>
>> - qualify_decode_pid(Yflag_qual);
>> + qualify_decode_pid(Yflag_short == 1 ? Yflag_qual : YYflag_qual);
>> }
>> /* If --decode-pids option comes after -p, comm fields of tcbs
>> * are not filled though tcbs are initialized. We must fill the
>> diff --git a/src/time.c b/src/time.c
>> index 2b0ded515..343342465 100644
>> --- a/src/time.c
>> +++ b/src/time.c
>> @@ -253,7 +253,9 @@ printclockname(int clockid)
>> tprints_arg_begin(CPUCLOCK_PERTHREAD(clockid) ?
>> "MAKE_THREAD_CPUCLOCK" :
>> "MAKE_PROCESS_CPUCLOCK");
>> - PRINT_VAL_D(CPUCLOCK_PID(clockid));
>> + int pid = CPUCLOCK_PID(clockid);
>> + PRINT_VAL_D(pid);
>> + maybe_printpid_comm(pid);
>> tprint_arg_next();
>> printxval(cpuclocknames, clockid & CLOCKFD_MASK,
>> "CPUCLOCK_???");
>
> I missed this occurence of PID printing after introducing the printpid
> function, but we should use printpid here as well.
>
>> diff --git a/src/util.c b/src/util.c
>> index a22d4095f..d40184963 100644
>> --- a/src/util.c
>> +++ b/src/util.c
>> @@ -642,6 +642,7 @@ printpidfd(pid_t pid_of_fd, int fd, const char *path)
>>
>> tprints("pid:");
>> PRINT_VAL_D(pid);
>> + maybe_printpid_comm(pid);
>> return true;
>> }
>
> This is correct, as this PID is always in strace's namespace. But to
> decrease code repetition a bit, we could also print this PID using
> printpid with a NULL tcb argument (which means that the PID is in
> strace's namespace).
>
More information about the Strace-devel
mailing list