From 500c3c17379fe0a62d42ba31bdcdb584b1823f60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20Sj=C3=B6len?= Date: Mon, 9 Jan 2023 10:18:26 +0000 Subject: [PATCH] 8298730: Refactor subsystem_file_line_contents and add docs and tests Reviewed-by: sgehwolf, iklam --- .../os/linux/cgroupSubsystem_linux.hpp | 124 ++++++----- .../os/linux/cgroupV1Subsystem_linux.cpp | 13 +- .../os/linux/cgroupV2Subsystem_linux.cpp | 2 +- .../os/linux/test_cgroupSubsystem_linux.cpp | 195 ++++++++++++++++++ .../gtest/runtime/test_os_linux_cgroups.cpp | 4 +- 5 files changed, 271 insertions(+), 67 deletions(-) create mode 100644 test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp diff --git a/src/hotspot/os/linux/cgroupSubsystem_linux.hpp b/src/hotspot/os/linux/cgroupSubsystem_linux.hpp index 91456b3d0e1..991f25b7172 100644 --- a/src/hotspot/os/linux/cgroupSubsystem_linux.hpp +++ b/src/hotspot/os/linux/cgroupSubsystem_linux.hpp @@ -78,71 +78,83 @@ class CgroupController: public CHeapObj { 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 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]; + 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 (file_path.size() > (MAXPATHLEN-1)) { + log_debug(os, container)("File path too long %s, %s", file_path.base(), filename); + return OSCONTAINER_ERROR; + } + 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; + } + + 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; + } + bool found_match = false; - - if (c == NULL) { - log_debug(os, container)("subsystem_file_line_contents: CgroupController* is NULL"); - return OSCONTAINER_ERROR; - } - if (c->subsystem_path() == NULL) { - log_debug(os, container)("subsystem_file_line_contents: subsystem path is NULL"); - 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); - 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 + 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 diff --git a/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp b/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp index d7dda076966..cfa8f516e3e 100644 --- a/src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp +++ b/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(_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,9 +121,8 @@ jlong CgroupV1Subsystem::memory_and_swap_limit_in_bytes() { CgroupV1MemoryController* mem_controller = reinterpret_cast(_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 { @@ -133,7 +130,7 @@ jlong CgroupV1Subsystem::memory_and_swap_limit_in_bytes() { 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); } diff --git a/src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp b/src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp index d200704e978..190b12438f6 100644 --- a/src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp +++ b/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); } diff --git a/test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp b/test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp new file mode 100644 index 00000000000..cfac19185a3 --- /dev/null +++ b/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 + + +// 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, (julong)1337) << "Wrong value for z"; +} + +#endif // LINUX diff --git a/test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp b/test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp index 9130c0c701d..917dd8bc40c 100644 --- a/test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp +++ b/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