On 4/26/22 03:45, Steven Rostedt wrote:
> On Fri, 22 Apr 2022 12:01:31 +0200
> Daniel Bristot de Oliveira <bristot@xxxxxxxxxx> 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;
>> +}
>> +
|