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 <[email protected]>
> ---
> 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
