Re: [PATCH 2/2] perf tool: Fix ppid for synthesized fork events

[Jiri Olsa <jolsa@redhat.com>]

On Sun, Mar 29, 2015 at 04:30:02PM -0600, David Ahern wrote:

SNIP

> -static pid_t perf_event__synthesize_comm(struct perf_tool *tool,
> -					 union perf_event *event, pid_t pid,
> -					 perf_event__handler_t process,
> -					 struct machine *machine)
> +static int perf_event__synthesize_comm(struct perf_tool *tool,
> +				       union perf_event *event, pid_t pid,
> +				       perf_event__handler_t process,
> +				       struct machine *machine,
> +				       pid_t *tgid, pid_t *ppid)
>  {
> -	pid_t tgid = perf_event__prepare_comm(event, pid, machine);
> -
> -	if (tgid == -1)
> -		goto out;
> +	if (perf_event__prepare_comm(event, pid, machine, tgid, ppid) != 0)
> +		return -1;

why dont we set ppid also for single comm event? seems to me
like following assignments:

        event->fork.ppid = ppid;
        event->fork.ptid = ppid;
        event->fork.pid  = tgid;
        event->fork.tid  = pid;
        event->fork.header.type = PERF_RECORD_FORK;

should be part of perf_event__prepare_comm function..?

SNIP

> @@ -365,14 +387,12 @@ static int __event__synthesize_thread(union perf_event *comm_event,
>  	char filename[PATH_MAX];
>  	DIR *tasks;
>  	struct dirent dirent, *next;
> -	pid_t tgid;
> +	pid_t tgid, ppid;
>  
>  	/* special case: only send one comm event using passed in pid */
>  	if (!full) {
> -		tgid = perf_event__synthesize_comm(tool, comm_event, pid,
> -						   process, machine);
> -
> -		if (tgid == -1)
> +		if (perf_event__synthesize_comm(tool, comm_event, pid,
> +						process, machine, &tgid, &ppid) != 0)

why did you add *ppid arg into perf_event__synthesize_comm?
I see no use for it in the rest of the caller..


jirka