* [PATCH] selftests/mm: read memory information without popen @ 2026-08-03 1:30 Warren Xiong 2026-08-04 11:15 ` David Hildenbrand (Arm) 2026-08-04 12:16 ` [PATCH v2] " Warren Xiong 0 siblings, 2 replies; 5+ messages in thread From: Warren Xiong @ 2026-08-03 1:30 UTC (permalink / raw) To: akpm, shuah Cc: david, ljs, liam, vbabka, rppt, surenb, mhocko, linux-mm, linux-kselftest, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] selftests/mm: read memory information without popen 2026-08-03 1:30 [PATCH] selftests/mm: read memory information without popen Warren Xiong @ 2026-08-04 11:15 ` David Hildenbrand (Arm) 2026-08-04 12:07 ` warren.xiong 2026-08-04 12:16 ` [PATCH v2] " Warren Xiong 1 sibling, 1 reply; 5+ messages in thread From: David Hildenbrand (Arm) @ 2026-08-04 11:15 UTC (permalink / raw) To: Warren Xiong, akpm, shuah Cc: ljs, liam, vbabka, rppt, surenb, mhocko, linux-mm, linux-kselftest, linux-kernel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] selftests/mm: read memory information without popen 2026-08-04 11:15 ` David Hildenbrand (Arm) @ 2026-08-04 12:07 ` warren.xiong 0 siblings, 0 replies; 5+ messages in thread From: warren.xiong @ 2026-08-04 12:07 UTC (permalink / raw) To: david, akpm, shuah Cc: ljs, liam, vbabka, rppt, surenb, mhocko, linux-mm, linux-kselftest, linux-kernel -------------- 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] selftests/mm: read memory information without popen 2026-08-03 1:30 [PATCH] selftests/mm: read memory information without popen Warren Xiong 2026-08-04 11:15 ` David Hildenbrand (Arm) @ 2026-08-04 12:16 ` Warren Xiong 2026-08-04 12:21 ` David Hildenbrand (Arm) 1 sibling, 1 reply; 5+ messages in thread From: Warren Xiong @ 2026-08-04 12:16 UTC (permalink / raw) To: akpm, shuah Cc: david, ljs, liam, vbabka, rppt, surenb, mhocko, linux-mm, linux-kselftest, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] selftests/mm: read memory information without popen 2026-08-04 12:16 ` [PATCH v2] " Warren Xiong @ 2026-08-04 12:21 ` David Hildenbrand (Arm) 0 siblings, 0 replies; 5+ messages in thread From: David Hildenbrand (Arm) @ 2026-08-04 12:21 UTC (permalink / raw) To: Warren Xiong, akpm, shuah Cc: ljs, liam, vbabka, rppt, surenb, mhocko, linux-mm, linux-kselftest, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-08-04 14:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-03 1:30 [PATCH] selftests/mm: read memory information without popen Warren Xiong 2026-08-04 11:15 ` David Hildenbrand (Arm) 2026-08-04 12:07 ` warren.xiong 2026-08-04 12:16 ` [PATCH v2] " Warren Xiong 2026-08-04 12:21 ` David Hildenbrand (Arm)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox