Feature requests: list of PIDS, and all threads

[George Spelvin]

Thank you very much!

> +static void process_opt_p_list(char *opt)
> +{
> +	while (*opt) {
> +		/*
> +		 * We accept -p PID,PID; -p "`pidof PROG`"; -p "`pgrep PROG`".
> +		 * pidof uses space as delim, pgrep uses newline. :(
> +		 */
> +		int pid;
> +		struct tcb *tcp;
> +		char *delim = opt + strcspn(opt, ", \n\t");
> +		char c = *delim;
> +
> +		*delim = '\0';
> +		pid = atoi(opt); /* TODO: stricter parsing of the number? */
> +		if (pid <= 0) {
> +			error_msg("Invalid process id: '%s'", opt);
> +			*delim = c;
> +			return;
> +		}
> +		if (pid == strace_tracer_pid) {
> +			error_msg("I'm sorry, I can't let you do that, Dave.");
> +			*delim = c;
> +			return;
> +		}
> +		*delim = c;
> +		tcp = alloc_tcb(pid, 0);
> +		tcp->flags |= TCB_ATTACHED;
> +		/*
> +		 * pflag_seen says how many PIDs we handled,
> +		 * not how many -p opts there were.
> +		 * Used to decide whether to print pid prefix in logs.
> +		 */
> +		pflag_seen++;
> +		if (c == '\0')
> +			break;
> +		opt = delim + 1;
> +	}
> +}
> +

I probably would have used strtoul(), to get automatic whitespace skipping:
(WARNING: Untested code.)

+static void process_opt_p_list(char *opt)
+{
+	do {
+		/*
+		 * We accept -p PID,PID; -p "`pidof PROG`"; -p "`pgrep PROG`".
+		 * pidof uses space as delim, pgrep uses newline. :(
+		 */
+		struct tcb *tcp;
+		char *q;
+		unsigned long pid = strtoul(opt, &q, 10);
+
+		if ((pid_t)pid <= 0 || (pid_t)pid != pid || q == opt) {
+			error_msg("Invalid process id: '%s'", opt);
+			return;
+		}
+		if (pid == strace_tracer_pid) {
+			error_msg("I'm sorry, I can't let you do that, Dave.");
+			return;
+		}
+		tcp = alloc_tcb(pid, 0);
+		tcp->flags |= TCB_ATTACHED;
+		/*
+		 * pflag_seen says how many PIDs we handled,
+		 * not how many -p opts there were.
+		 * Used to decide whether to print pid prefix in logs.
+		 */
+		pflag_seen++;
+
+		opt = q + strspn(q, ", \n\t");
+	} while (*opt);
+}
+

An interesting question is whether zero pids in the PID list should be
permitted.  I changed the loop above to forbid it.

I'd also recommend updating the documentation:

diff --git a/strace.1 b/strace.1
index a98b9f9..a5c98ad 100644
--- a/strace.1
+++ b/strace.1
@@ -531,9 +531,11 @@ at any time by a keyboard interrupt signal (\c
 .B strace
 will respond by detaching itself from the traced process(es)
 leaving it (them) to continue running.
-Multiple
+Up to 32 pids may be listed, using either multiple 
 .B \-p
-options can be used to attach to up to 32 processes in addition to
+options, or a comma or whitespace separated list to a single
+.B \-p
+option, in addition to the
 .I command
 (which is optional if at least one
 .B \-p

(Both contributions released to the public domain, or licensed under CC0
in the alternative, I fart in the general direction of copyright lawyers,
yadda yadda.)

Some documentation for the all-threads behaviour of -f would also be
nice, but I'm not quite certain I understand it well enough to document
it accurately.  It seems to depend on LINUX && !daemonized_tracer.

I have no idea why the latter is an issue; when strace attaches in the
-D case, the target process is single-threaded.  I'm also not sure how
the code avoids attaching to the master task twice; It seems to attach
to /proc/$$/task/*, which normally includes /proc/$$/task/$$, and then
to $$ again.