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

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

[Warren Xiong]

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 <warren.xiong@ugreen.com>
---
 tools/testing/selftests/mm/compaction_test.c | 43 +++++++++++++++++-----------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c
index 5b58258..0df2000 100644
--- a/tools/testing/selftests/mm/compaction_test.c
+++ b/tools/testing/selftests/mm/compaction_test.c
@@ -7,6 +7,7 @@
  * allocated.
  */
 
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/mman.h>
@@ -29,30 +30,40 @@ 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];
+	bool memfree_found = false;
+	bool hugepagesize_found = false;
+	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);
+	while (fgets(buffer, sizeof(buffer), file)) {
+		if (sscanf(buffer, "MemFree: %lu kB", memfree) == 1)
+			memfree_found = true;
+		else if (sscanf(buffer, "Hugepagesize: %lu kB",
+				hugepagesize) == 1)
+			hugepagesize_found = true;
 
-	*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;
+		if (memfree_found && hugepagesize_found) {
+			ret = 0;
+			break;
+		}
 	}
 
-	pclose(cmdfile);
-	*hugepagesize = atoll(buffer);
+	if (ferror(file))
+		ksft_print_msg("Failed to read /proc/meminfo: %s\n",
+			       strerror(errno));
+	else if (ret)
+		ksft_print_msg("Failed to parse /proc/meminfo\n");
 
-	return 0;
+	fclose(file);
+	return ret;
 }
 
 int prereq(void)
-- 
2.7.4

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

[David Hildenbrand (Arm)]

On 8/3/26 03:30, 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 <warren.xiong@ugreen.com>
> ---
>  tools/testing/selftests/mm/compaction_test.c | 43 +++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c
> index 5b58258..0df2000 100644
> --- a/tools/testing/selftests/mm/compaction_test.c
> +++ b/tools/testing/selftests/mm/compaction_test.c
> @@ -7,6 +7,7 @@
>   * allocated.
>   */
>  
> +#include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <sys/mman.h>
> @@ -29,30 +30,40 @@ struct map_list {
>  
>  int read_memory_info(unsigned long *memfree, unsigned long *hugepagesize)
>  {

Looks much better indeed.

> -	char  buffer[256] = {0};
> -	char *cmd = "cat /proc/meminfo | grep -i memfree | grep -o '[0-9]*'";
> -	FILE *cmdfile = popen(cmd, "r");
> +	char buffer[256];
> +	bool memfree_found = false;
> +	bool hugepagesize_found = false;
> +	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);
> +	while (fgets(buffer, sizeof(buffer), file)) {
> +		if (sscanf(buffer, "MemFree: %lu kB", memfree) == 1)
> +			memfree_found = true;
> +		else if (sscanf(buffer, "Hugepagesize: %lu kB",
> +				hugepagesize) == 1)
> +			hugepagesize_found = true;
Instead of these bools you could just have a hit-count.

Like the following on top:

diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c
index 0df2000f4a2b1..30d4ace7155ae 100644
--- a/tools/testing/selftests/mm/compaction_test.c
+++ b/tools/testing/selftests/mm/compaction_test.c
@@ -7,7 +7,6 @@
  * allocated.
  */

-#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/mman.h>
@@ -31,8 +30,7 @@ struct map_list {
 int read_memory_info(unsigned long *memfree, unsigned long *hugepagesize)
 {
        char buffer[256];
-       bool memfree_found = false;
-       bool hugepagesize_found = false;
+       int found = 0;
        FILE *file;
        int ret = -1;

@@ -43,24 +41,19 @@ int read_memory_info(unsigned long *memfree, unsigned long *hugepagesize)
                return -1;
        }

-       while (fgets(buffer, sizeof(buffer), file)) {
-               if (sscanf(buffer, "MemFree: %lu kB", memfree) == 1)
-                       memfree_found = true;
-               else if (sscanf(buffer, "Hugepagesize: %lu kB",
-                               hugepagesize) == 1)
-                       hugepagesize_found = true;
-
-               if (memfree_found && hugepagesize_found) {
-                       ret = 0;
-                       break;
-               }
+       while (fgets(buffer, sizeof(buffer), file) && found != 2) {
+               if (sscanf(buffer, "MemFree: %lu kB", memfree) == 1 ||
+                   sscanf(buffer, "Hugepagesize: %lu kB", hugepagesize) == 1)
+                       found++;
        }

        if (ferror(file))
                ksft_print_msg("Failed to read /proc/meminfo: %s\n",
                               strerror(errno));
-       else if (ret)
+       else if (found != 2)
                ksft_print_msg("Failed to parse /proc/meminfo\n");
+       else
+               ret = 0;

        fclose(file);
        return ret;


-- 
Cheers,

David

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

[warren.xiong]

--------------



warren.xiong



>On 8/3/26 03:30, 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 <warren.xiong@ugreen.com>



>> ---



>>  tools/testing/selftests/mm/compaction_test.c | 43 +++++++++++++++++-----------



>>  1 file changed, 27 insertions(+), 16 deletions(-)



>> 



>> diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c



>> index 5b58258..0df2000 100644



>> --- a/tools/testing/selftests/mm/compaction_test.c



>> +++ b/tools/testing/selftests/mm/compaction_test.c



>> @@ -7,6 +7,7 @@



>>   * allocated.



>>   */



>>  



>> +#include <stdbool.h>



>>  #include <stdio.h>



>>  #include <stdlib.h>



>>  #include <sys/mman.h>



>> @@ -29,30 +30,40 @@ struct map_list {



>>  



>>  int read_memory_info(unsigned long *memfree, unsigned long *hugepagesize)



>>  {



>



>Looks much better indeed.



>



>> -	char  buffer[256] = {0};



>> -	char *cmd = "cat /proc/meminfo | grep -i memfree | grep -o '[0-9]*'";



>> -	FILE *cmdfile = popen(cmd, "r");



>> +	char buffer[256];



>> +	bool memfree_found = false;



>> +	bool hugepagesize_found = false;



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



>> +	while (fgets(buffer, sizeof(buffer), file)) {



>> +		if (sscanf(buffer, "MemFree: %lu kB", memfree) == 1)



>> +			memfree_found = true;



>> +		else if (sscanf(buffer, "Hugepagesize: %lu kB",



>> +				hugepagesize) == 1)



>> +			hugepagesize_found = true;



>Instead of these bools you could just have a hit-count.

Thanks for the review.

Agreed, a hit count is sufficient here and makes the code simpler.  I
have incorporated this change and will send v2 shortly.

Thanks,
Warren



>



>Like the following on top:



>



>diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c



>index 0df2000f4a2b1..30d4ace7155ae 100644



>--- a/tools/testing/selftests/mm/compaction_test.c



>+++ b/tools/testing/selftests/mm/compaction_test.c



>@@ -7,7 +7,6 @@



>  * allocated.



>  */



>



>-#include <stdbool.h>



> #include <stdio.h>



> #include <stdlib.h>



> #include <sys/mman.h>



>@@ -31,8 +30,7 @@ struct map_list {



> int read_memory_info(unsigned long *memfree, unsigned long *hugepagesize)



> {



>        char buffer[256];



>-       bool memfree_found = false;



>-       bool hugepagesize_found = false;



>+       int found = 0;



>        FILE *file;



>        int ret = -1;



>



>@@ -43,24 +41,19 @@ int read_memory_info(unsigned long *memfree, unsigned long *hugepagesize)



>                return -1;



>        }



>



>-       while (fgets(buffer, sizeof(buffer), file)) {



>-               if (sscanf(buffer, "MemFree: %lu kB", memfree) == 1)



>-                       memfree_found = true;



>-               else if (sscanf(buffer, "Hugepagesize: %lu kB",



>-                               hugepagesize) == 1)



>-                       hugepagesize_found = true;



>-



>-               if (memfree_found && hugepagesize_found) {



>-                       ret = 0;



>-                       break;



>-               }



>+       while (fgets(buffer, sizeof(buffer), file) && found != 2) {



>+               if (sscanf(buffer, "MemFree: %lu kB", memfree) == 1 ||



>+                   sscanf(buffer, "Hugepagesize: %lu kB", hugepagesize) == 1)



>+                       found++;



>        }



>



>        if (ferror(file))



>                ksft_print_msg("Failed to read /proc/meminfo: %s\n",



>                               strerror(errno));



>-       else if (ret)



>+       else if (found != 2)



>                ksft_print_msg("Failed to parse /proc/meminfo\n");



>+       else



>+               ret = 0;



>



>        fclose(file);



>        return ret;



>



>



>-- 



>Cheers,



>



>David



>

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

[Warren Xiong]

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 <warren.xiong@ugreen.com>
---
Changes in v2:
- Replace the two boolean flags with a hit counter, as suggested by
  David Hildenbrand.

v1: https://lore.kernel.org/1785720615-5826-1-git-send-email-warren.xiong@ugreen.com/

 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++;
 	}
 
-	pclose(cmdfile);
-	*hugepagesize = atoll(buffer);
+	if (ferror(file))
+		ksft_print_msg("Failed to read /proc/meminfo: %s\n",
+			       strerror(errno));
+	else if (found != 2)
+		ksft_print_msg("Failed to parse /proc/meminfo\n");
+	else
+		ret = 0;
 
-	return 0;
+	fclose(file);
+	return ret;
 }
 
 int prereq(void)
-- 
2.7.4

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

[David Hildenbrand (Arm)]

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 <warren.xiong@ugreen.com>
> ---
> Changes in v2:
> - Replace the two boolean flags with a hit counter, as suggested by
>   David Hildenbrand.
> 
> v1: https://lore.kernel.org/1785720615-5826-1-git-send-email-warren.xiong@ugreen.com/
> 
>  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) <david@kernel.org>

-- 
Cheers,

David