8298730: Refactor subsystem_file_line_contents and add docs and tests

Reviewed-by: phh
Backport-of: 500c3c17379fe0a62d42ba31bdcdb584b1823f60

Author: GoeLin

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 = 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 = 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

@@ -224,7 +224,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);
 }
 

src/hotspot/share/utilities/ostream.cpp

@@ -354,6 +354,7 @@ void stringStream::grow(size_t new_capacity) {
 }
 
 void stringStream::write(const char* s, size_t len) {
+  assert(_is_frozen == false, "Modification forbidden");
   assert(_capacity >= _written + 1, "Sanity");
   if (len == 0) {
     return;
@@ -393,6 +394,7 @@ void stringStream::zero_terminate() {
 }
 
 void stringStream::reset() {
+  assert(_is_frozen == false, "Modification forbidden");
   _written = 0; _precount = 0; _position = 0;
   _newlines = 0;
   zero_terminate();

src/hotspot/share/utilities/ostream.hpp

@@ -28,6 +28,7 @@
 #include "memory/allocation.hpp"
 #include "runtime/timer.hpp"
 #include "utilities/globalDefinitions.hpp"
+#include "utilities/macros.hpp"
 
 DEBUG_ONLY(class ResourceMark;)
 
@@ -191,6 +192,7 @@ class ttyUnlocker: StackObj {
 // for writing to strings; buffer will expand automatically.
 // Buffer will always be zero-terminated.
 class stringStream : public outputStream {
+  DEBUG_ONLY(bool _is_frozen = false);
   char*  _buffer;
   size_t _written;  // Number of characters written, excluding termin. zero
   size_t _capacity;
@@ -215,9 +217,18 @@ class stringStream : public outputStream {
   // Return number of characters written into buffer, excluding terminating zero and
   // subject to truncation in static buffer mode.
   size_t      size() const { return _written; }
+  // Returns internal buffer containing the accumulated string.
+  // Returned buffer is only guaranteed to be valid as long as stream is not modified
   const char* base() const { return _buffer; }
+  // Freezes stringStream (no further modifications possible) and returns pointer to it.
+  // No-op if stream is frozen already.
+  // Returns the internal buffer containing the accumulated string.
+  const char* freeze() NOT_DEBUG(const) {
+    DEBUG_ONLY(_is_frozen = true);
+    return _buffer;
+  };
   void  reset();
-  // copy to a resource, or C-heap, array as requested
+  // Copy to a resource, or C-heap, array as requested
   char* as_string(bool c_heap = false) const;
 };