Re: [PATCH v2] selftests/mm: read memory information without popen

On 8/4/26 14:16, Warren Xiong wrote:
> read_memory_info() invokes two shell pipelines to obtain MemFree and
> Hugepagesize from /proc/meminfo. It does not check whether popen()
> returns NULL before passing the result to fgets(), and it does not call
> pclose() when fgets() fails.
> 
> Open /proc/meminfo directly and obtain both values in a single pass.
> This removes the unchecked NULL path, closes the file on all paths, and
> avoids dependencies on external commands.
> 
> The compaction test continues to pass after this change.
> 
> Signed-off-by: Warren Xiong <[email protected]>
> ---
> Changes in v2:
> - Replace the two boolean flags with a hit counter, as suggested by
>   David Hildenbrand.
> 
> v1: https://lore.kernel.org/[email protected]/
> 
>  tools/testing/selftests/mm/compaction_test.c | 38 +++++++++++++++-------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c
> index 5b58258..30d4ace 100644
> --- a/tools/testing/selftests/mm/compaction_test.c
> +++ b/tools/testing/selftests/mm/compaction_test.c
> @@ -29,30 +29,34 @@ struct map_list {
>  
>  int read_memory_info(unsigned long *memfree, unsigned long *hugepagesize)
>  {
> -	char  buffer[256] = {0};
> -	char *cmd = "cat /proc/meminfo | grep -i memfree | grep -o '[0-9]*'";
> -	FILE *cmdfile = popen(cmd, "r");
> +	char buffer[256];
> +	int found = 0;
> +	FILE *file;
> +	int ret = -1;
>  
> -	if (!(fgets(buffer, sizeof(buffer), cmdfile))) {
> -		ksft_print_msg("Failed to read meminfo: %s\n", strerror(errno));
> +	file = fopen("/proc/meminfo", "r");
> +	if (!file) {
> +		ksft_print_msg("Failed to open /proc/meminfo: %s\n",
> +			       strerror(errno));
>  		return -1;
>  	}
>  
> -	pclose(cmdfile);
> -
> -	*memfree = atoll(buffer);
> -	cmd = "cat /proc/meminfo | grep -i hugepagesize | grep -o '[0-9]*'";
> -	cmdfile = popen(cmd, "r");
> -
> -	if (!(fgets(buffer, sizeof(buffer), cmdfile))) {
> -		ksft_print_msg("Failed to read meminfo: %s\n", strerror(errno));
> -		return -1;
> +	while (fgets(buffer, sizeof(buffer), file) && found != 2) {
> +		if (sscanf(buffer, "MemFree: %lu kB", memfree) == 1 ||
> +		    sscanf(buffer, "Hugepagesize: %lu kB", hugepagesize) == 1)
> +			found++;

Just as information: when proposing that I was assuming that we would never ever
have two times the same entry in /proc/meminfo, which i think we can reasonably
assume.

Thanks!

Acked-by: David Hildenbrand (Arm) <[email protected]>

-- 
Cheers,

David