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
