Re: [PATCH 1/2] rtla: Remove procps-ng dependency

[Daniel Bristot de Oliveira <bristot@kernel.org>]

On 4/26/22 03:45, Steven Rostedt wrote:
> On Fri, 22 Apr 2022 12:01:31 +0200
> Daniel Bristot de Oliveira <bristot@kernel.org> wrote:
> 
> 
>> +
>> +/*
>> + * procfs_is_workload_pid - check if a procfs entry contains a workload_prefix* comm
>> + *
>> + * Check if the procfs entry is a directory of a process, and then check if the
>> + * process has a comm with the prefix set in char *workload_prefix. As the
>> + * current users of this function only check for kernel threads, there is no
>> + * need to check for the threads for the process.
>> + *
>> + * Return: True if the proc_entry contains a comm file with workload_prefix*.
>> + * Otherwise returns false.
>> + */
>> +static int procfs_is_workload_pid(const char *workload_prefix, struct dirent *proc_entry)
>> +{
>> +	char comm_path[MAX_PATH], comm[MAX_PATH];
> 
> This is probably fine (but there is one issue), but I would have done this
> a little different.
> 
> 	int len = strlen(workload_prefix);
> 	char comm[len + 1];
> 
>> +	int comm_fd, retval;
>> +	char *t_name;
>> +
>> +	if (proc_entry->d_type != DT_DIR)
>> +		return 0;
>> +
>> +	if (*proc_entry->d_name == '.')
>> +		return 0;
>> +
>> +	/* check if the string is a pid */
>> +	for (t_name = proc_entry->d_name; t_name; t_name++) {
>> +		if (!isdigit(*t_name))
>> +			break;
>> +	}
>> +
>> +	if (*t_name != '\0')
>> +		return 0;
>> +
>> +	snprintf(comm_path, MAX_PATH, "/proc/%s/comm", proc_entry->d_name);
>> +	comm_fd = open(comm_path, O_RDONLY);
>> +	if (comm_fd < 0)
>> +		return 0;
>> +
>> +	memset(comm, 0, MAX_PATH);
> 
> No need for the memset.
> 
>> +	retval = read(comm_fd, comm, MAX_PATH);
> 
> 	retval = read(comm_fd, comm, len + 1);
> 
>> +
>> +	close(comm_fd);
>> +
>> +	if (retval <= 0)
>> +		return 0;
> 
> 	if (comm[len] != '\n')
> 		return 0;
> 
>> +
>> +	retval = !strncmp(workload_prefix, comm, strlen(workload_prefix));
> 
> What happens if strlen(workload_prefix) is greater than MAX_PATH? ;-)

That is a concern I had, but we have only two use cases on rtla so far, and they
are both bounded: 'timerlat/' 'osnoise/'...

I will add a check though.

> 
> 	retval = !strncmp(workload_prefix, comm, len);
> 
> But that's me. If you want to keep this as is, let me know.

I made these changes... but I am loosing the debug_msg() with the comm/pid that
matched. I am adding these messages (and I plan to add more) so I can use on
testing scripts...

At the same time, I can share a char buffer[MAX_PATH] for both the comm and
comm_path...

I think I will go this way (using a single buffer), adding a warning... and...

> -- Steve
> 
> 
> 
>> +	if (!retval)
>> +		return 0;

also removing ! in the the revtal = !strcmp(...); if (!retval).

Thoughts?

-- Daniel

>> +
>> +	/* comm already have \n */
>> +	debug_msg("Found workload pid:%s comm:%s", proc_entry->d_name, comm);
>> +
>> +	return 1;
>> +}
>> +