Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1535620a-5a41-1ec4-3963-c9bc3b8e117f@kernel.org>
Date:   Tue, 26 Apr 2022 13:57:13 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH 1/2] rtla: Remove procps-ng dependency
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-trace-devel@vger.kernel.org, linux-kernel@vger.kernel.org,
        John Kacur <jkacur@redhat.com>, Daniel Wagner <dwagner@suse.de>
References: <52161d343abeda6d039d2f0734904a1383a4c62e.1650617571.git.bristot@kernel.org>
 <20220425214559.55988956@gandalf.local.home>
From:   Daniel Bristot de Oliveira <bristot@kernel.org>
In-Reply-To: <20220425214559.55988956@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;
>> +}
>> +