8298730: Refactor subsystem_file_line_contents and add docs and tests

src/hotspot/os/linux/cgroupSubsystem_linux.hpp

@@ -78,71 +78,83 @@ class CgroupController: public CHeapObj<mtInternal> {
 
 PRAGMA_DIAG_PUSH
 PRAGMA_FORMAT_NONLITERAL_IGNORED
+// Parses a subsystem's file, looking for a matching line.
+// If key is null, then the first line will be matched with scan_fmt.
+// If key isn't null, then each line will be matched, looking for something that matches "$key $scan_fmt".
+// The matching value will be assigned to returnval.
+// scan_fmt uses scanf() syntax.
+// Return value: 0 on match, OSCONTAINER_ERROR on error.
 template <typename T> int subsystem_file_line_contents(CgroupController* c,
                                               const char *filename,
-                                              const char *matchline,
+                                              const char *key,
                                               const char *scan_fmt,
                                               T returnval) {
-  FILE *fp = NULL;
-  char *p;
-  char file[MAXPATHLEN+1];
-  char buf[MAXPATHLEN+1];
-  char discard[MAXPATHLEN+1];
-  bool found_match = false;
+  if (c == nullptr) {
+    log_debug(os, container)("subsystem_file_line_contents: CgroupController* is nullptr");
+    return OSCONTAINER_ERROR;
+  }
+  if (c->subsystem_path() == nullptr) {
+    log_debug(os, container)("subsystem_file_line_contents: subsystem path is nullptr");
+    return OSCONTAINER_ERROR;
+  }
+
+  stringStream file_path;
+  file_path.print_raw(c->subsystem_path());
+  file_path.print_raw(filename);
 
-  if (c == NULL) {
-    log_debug(os, container)("subsystem_file_line_contents: CgroupController* is NULL");
+  if (file_path.size() > (MAXPATHLEN-1)) {
+    log_debug(os, container)("File path too long %s, %s", file_path.base(), filename);
     return OSCONTAINER_ERROR;
   }
-  if (c->subsystem_path() == NULL) {
-    log_debug(os, container)("subsystem_file_line_contents: subsystem path is NULL");
+  const char* absolute_path = file_path.freeze();
+  log_trace(os, container)("Path to %s is %s", filename, absolute_path);
+
+  FILE* fp = os::fopen(absolute_path, "r");
+  if (fp == nullptr) {
+    log_debug(os, container)("Open of file %s failed, %s", absolute_path, os::strerror(errno));
     return OSCONTAINER_ERROR;
   }
 
-  strncpy(file, c->subsystem_path(), MAXPATHLEN);
-  file[MAXPATHLEN-1] = '\0';
-  int filelen = strlen(file);
-  if ((filelen + strlen(filename)) > (MAXPATHLEN-1)) {
-    log_debug(os, container)("File path too long %s, %s", file, filename);
+  const int buf_len = MAXPATHLEN+1;
+  char buf[buf_len];
+  char* line = fgets(buf, buf_len, fp);
+  if (line == nullptr) {
+    log_debug(os, container)("Empty file %s", absolute_path);
+    fclose(fp);
     return OSCONTAINER_ERROR;
   }
-  strncat(file, filename, MAXPATHLEN-filelen);
-  log_trace(os, container)("Path to %s is %s", filename, file);
-  fp = os::fopen(file, "r");
-  if (fp != NULL) {
-    int err = 0;
-    while ((p = fgets(buf, MAXPATHLEN, fp)) != NULL) {
-      found_match = false;
-      if (matchline == NULL) {
-        // single-line file case
-        int matched = sscanf(p, scan_fmt, returnval);
-        found_match = (matched == 1);
-      } else {
-        // multi-line file case
-        if (strstr(p, matchline) != NULL) {
-          // discard matchline string prefix
-          int matched = sscanf(p, scan_fmt, discard, returnval);
-          found_match = (matched == 2);
-        } else {
-          continue; // substring not found
+
+  bool found_match = false;
+  if (key == nullptr) {
+    // File consists of a single line according to caller, with only a value
+    int matched = sscanf(line, scan_fmt, returnval);
+    found_match = matched == 1;
+  } else {
+    // File consists of multiple lines in a "key value"
+    // fashion, we have to find the key.
+    const int key_len = strlen(key);
+    for (; line != nullptr; line = fgets(buf, buf_len, fp)) {
+      char* key_substr = strstr(line, key);
+      char after_key = line[key_len];
+      if (key_substr == line
+          && isspace(after_key) != 0
+          && after_key != '\n') {
+        // Skip key, skip space
+        const char* value_substr = line + key_len + 1;
+        int matched = sscanf(value_substr, scan_fmt, returnval);
+        found_match = matched == 1;
+        if (found_match) {
+          break;
         }
       }
-      if (found_match) {
-        fclose(fp);
-        return 0;
-      } else {
-        err = 1;
-        log_debug(os, container)("Type %s not found in file %s", scan_fmt, file);
-      }
     }
-    if (err == 0) {
-      log_debug(os, container)("Empty file %s", file);
-    }
-  } else {
-    log_debug(os, container)("Open of file %s failed, %s", file, os::strerror(errno));
   }
-  if (fp != NULL)
-    fclose(fp);
+  fclose(fp);
+  if (found_match) {
+    return 0;
+  }
+  log_debug(os, container)("Type %s (key == %s) not found in file %s", scan_fmt,
+                           (key == nullptr ? "null" : key), absolute_path);
   return OSCONTAINER_ERROR;
 }
 PRAGMA_DIAG_POP

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp

@@ -96,10 +96,8 @@ jlong CgroupV1Subsystem::read_memory_limit_in_bytes() {
     log_trace(os, container)("Non-Hierarchical Memory Limit is: Unlimited");
     CgroupV1MemoryController* mem_controller = reinterpret_cast<CgroupV1MemoryController*>(_memory->controller());
     if (mem_controller->is_hierarchical()) {
-      const char* matchline = "hierarchical_memory_limit";
-      const char* format = "%s " JULONG_FORMAT;
-      GET_CONTAINER_INFO_LINE(julong, _memory->controller(), "/memory.stat", matchline,
-                             "Hierarchical Memory Limit is: " JULONG_FORMAT, format, hier_memlimit)
+      GET_CONTAINER_INFO_LINE(julong, _memory->controller(), "/memory.stat", "hierarchical_memory_limit",
+                             "Hierarchical Memory Limit is: " JULONG_FORMAT, JULONG_FORMAT, hier_memlimit)
       if (hier_memlimit >= os::Linux::physical_memory()) {
         log_trace(os, container)("Hierarchical Memory Limit is: Unlimited");
       } else {
@@ -123,17 +121,16 @@ jlong CgroupV1Subsystem::memory_and_swap_limit_in_bytes() {
     CgroupV1MemoryController* mem_controller = reinterpret_cast<CgroupV1MemoryController*>(_memory->controller());
     if (mem_controller->is_hierarchical()) {
       const char* matchline = "hierarchical_memsw_limit";
-      const char* format = "%s " JULONG_FORMAT;
       GET_CONTAINER_INFO_LINE(julong, _memory->controller(), "/memory.stat", matchline,
-                             "Hierarchical Memory and Swap Limit is : " JULONG_FORMAT, format, hier_memswlimit)
+                             "Hierarchical Memory and Swap Limit is : " JULONG_FORMAT, JULONG_FORMAT, hier_memswlimit)
       if (hier_memswlimit >= host_total_memsw) {
         log_trace(os, container)("Hierarchical Memory and Swap Limit is: Unlimited");
       } else {
         jlong swappiness = read_mem_swappiness();
         if (swappiness == 0) {
             const char* matchmemline = "hierarchical_memory_limit";
             GET_CONTAINER_INFO_LINE(julong, _memory->controller(), "/memory.stat", matchmemline,
-                             "Hierarchical Memory Limit is : " JULONG_FORMAT, format, hier_memlimit)
+                             "Hierarchical Memory Limit is : " JULONG_FORMAT, JULONG_FORMAT, hier_memlimit)
             log_trace(os, container)("Memory and Swap Limit has been reset to " JULONG_FORMAT " because swappiness is 0", hier_memlimit);
             return (jlong)hier_memlimit;
         }
@@ -286,7 +283,7 @@ int CgroupV1Subsystem::cpu_shares() {
 
 char* CgroupV1Subsystem::pids_max_val() {
   GET_CONTAINER_INFO_CPTR(cptr, _pids, "/pids.max",
-                     "Maximum number of tasks is: %s", "%s %*d", pidsmax, 1024);
+                     "Maximum number of tasks is: %s", "%1023s", pidsmax, 1024);
   return os::strdup(pidsmax);
 }
 

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp

@@ -223,7 +223,7 @@ char* CgroupV2Controller::construct_path(char* mount_path, char *cgroup_path) {
 
 char* CgroupV2Subsystem::pids_max_val() {
   GET_CONTAINER_INFO_CPTR(cptr, _unified, "/pids.max",
-                     "Maximum number of tasks is: %s", "%1023s %*d", pidsmax, 1024);
+                     "Maximum number of tasks is: %s", "%1023s", pidsmax, 1024);
   return os::strdup(pidsmax);
 }
 

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp

@@ -0,0 +1,195 @@
+/*
+ * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include "precompiled.hpp"
+
+#ifdef LINUX
+
+#include "runtime/os.hpp"
+#include "cgroupSubsystem_linux.hpp"
+#include "unittest.hpp"
+#include "utilities/globalDefinitions.hpp"
+
+#include <stdio.h>
+
+
+// Utilities
+bool file_exists(const char* filename) {
+  struct stat st;
+  return os::stat(filename, &st) == 0;
+}
+
+char* temp_file(const char* prefix) {
+  const testing::TestInfo* test_info = ::testing::UnitTest::GetInstance()->current_test_info();
+  stringStream path;
+  path.print_raw(os::get_temp_directory());
+  path.print_raw(os::file_separator());
+  path.print("%s-test-jdk.pid%d.%s.%s", prefix, os::current_process_id(),
+             test_info->test_case_name(), test_info->name());
+  return path.as_string(true);
+}
+
+void delete_file(const char* filename) {
+  if (!file_exists(filename)) {
+    return;
+  }
+  int ret = remove(filename);
+  EXPECT_TRUE(ret == 0 || errno == ENOENT) << "failed to remove file '" << filename << "': "
+      << os::strerror(errno) << " (" << errno << ")";
+}
+
+class TestController : public CgroupController {
+public:
+  char* subsystem_path() override {
+    // The real subsystem is in /tmp/, generaed by temp_file()
+    return (char*)"/";
+  };
+};
+
+void fill_file(const char* path, const char* content) {
+  delete_file(path);
+  FILE* fp = os::fopen(path, "w");
+  if (fp == nullptr) {
+    return;
+  }
+  if (content != nullptr) {
+    fprintf(fp, "%s", content);
+  }
+  fclose(fp);
+}
+
+TEST(cgroupTest, SubSystemFileLineContentsMultipleLinesErrorCases) {
+  TestController my_controller{};
+  const char* test_file = temp_file("cgroups");
+  int x = 0;
+  char s[1024];
+  int err = 0;
+
+  s[0] = '\0';
+  fill_file(test_file, "foo ");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_NE(err, 0) << "Value must not be missing in key/value case";
+
+  s[0] = '\0';
+  fill_file(test_file, "faulty_start foo bar");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_NE(err, 0) << "Key must be at start";
+
+  s[0] = '\0';
+  fill_file(test_file, "foof bar");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_NE(err, 0) << "Key must be exact match";
+}
+
+TEST(cgroupTest, SubSystemFileLineContentsMultipleLinesSuccessCases) {
+  TestController my_controller{};
+  const char* test_file = temp_file("cgroups");
+  int x = 0;
+  char s[1024];
+  int err = 0;
+
+  s[0] = '\0';
+  fill_file(test_file, "foo bar");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "bar") << "Incorrect!";
+
+  s[0] = '\0';
+  fill_file(test_file, "foo\tbar");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "bar") << "Incorrect!";
+
+  s[0] = '\0';
+  fill_file(test_file, "foof bar\nfoo car");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "car");
+
+  s[0] = '\0';
+  fill_file(test_file, "foo\ttest\nfoot car");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "test");
+
+  s[0] = '\0';
+  fill_file(test_file, "foo 1\nfoo car");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "1");
+
+  s[0] = '\0';
+  fill_file(test_file, "max 10000");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%s %*d", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "max");
+
+  x = -3;
+  fill_file(test_file, "max 10001");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%*s %d", &x);
+  EXPECT_EQ(err, 0);
+  EXPECT_EQ(x, 10001);
+}
+
+TEST(cgroupTest, SubSystemFileLineContentsSingleLine) {
+  TestController my_controller{};
+  const char* test_file = temp_file("cgroups");
+  int x = 0;
+  char s[1024];
+  int err = 0;
+
+  fill_file(test_file, "foo");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%s", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "foo");
+
+  fill_file(test_file, "1337");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%d", &x);
+  EXPECT_EQ(err, 0);
+  EXPECT_EQ(x, 1337) << "Wrong value for x";
+
+  s[0] = '\0';
+  fill_file(test_file, "1337");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%s", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "1337");
+
+  x = -1;
+  fill_file(test_file, nullptr);
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%d", &x);
+  EXPECT_NE(err, 0) << "Empty file should've failed";
+  EXPECT_EQ(x, -1) << "x was altered";
+
+  jlong y;
+  fill_file(test_file, "1337");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, JLONG_FORMAT, &y);
+  EXPECT_EQ(err, 0);
+  EXPECT_EQ(y, 1337) << "Wrong value for y";
+  julong z;
+  fill_file(test_file, "1337");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, JULONG_FORMAT, &z);
+  EXPECT_EQ(err, 0);
+  EXPECT_EQ(z, 1337) << "Wrong value for z";
+}
+
+#endif // LINUX

test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp

@@ -36,7 +36,7 @@ typedef struct {
   const char* expected_path;
 } TestCase;
 
-TEST(os_linux_cgroup, set_cgroupv1_subsystem_path) {
+TEST(cgroupTest, set_cgroupv1_subsystem_path) {
   TestCase host = {
     "/sys/fs/cgroup/memory",                                             // mount_path
     "/",                                                                 // root_path
@@ -60,7 +60,7 @@ TEST(os_linux_cgroup, set_cgroupv1_subsystem_path) {
   }
 }
 
-TEST(os_linux_cgroup, set_cgroupv2_subsystem_path) {
+TEST(cgroupTest, set_cgroupv2_subsystem_path) {
   TestCase at_mount_root = {
     "/sys/fs/cgroup",       // mount_path
     NULL,                   // root_path, ignored