Add an API for system processes #7681

harship04 · 2025-03-07T15:31:11Z

Use /proc followed by iterating the directories of each process and extracted the command line. Used cmdline for user space processes and comm for others.

babsingh · 2025-03-07T16:07:46Z

@keithc-ca could you please help in reviewing this PR?

keithc-ca · 2025-03-07T21:29:57Z

port/unix/omrsysinfo.c

+        DIR *dir = opendir("/proc");
+        if (!dir) {
+                int32_t rc = findError(errno);
+                portLibrary->error_set_last_error(portLibrary, errno, rc);
+                Trc_PRT_sysinfo_get_open_file_count_failedOpeningProcFS(rc); 
+                return -1; 
+        }
+        struct dirent *entry;
+        char cmdline_path[PATH_MAX];
+        char comm_path[PATH_MAX];
+        char command[PATH_MAX];
+        size_t bytes_read;
+        while ((entry = readdir(dir)) != NULL) {


This is not strictly legal C code: declarations must be at the beginning of a block.

Please first update this so the additions follow the coding standard. Some examples of things that don't follow those guidelines:

comparisons (== and != where the constant is not on the left

conditions with embedded assignments

uninitialized local variables (e.g. entry, bytes_read above)

code should be indented one tab per level (not 8 spaces)

missing comments on #else and #endif

I have made the changes given by you and updated PR

keithc-ca · 2025-03-12T20:04:23Z

port/common/omrport.c

@@ -85,6 +85,7 @@ static OMRPortLibrary MainPortLibraryTable = {
 	omrsysinfo_get_load_average, /* sysinfo_get_load_average */
 	omrsysinfo_get_CPU_utilization, /* sysinfo_get_CPU_utilization */
 	omrsysinfo_get_CPU_load, /* sysinfo_get_CPU_load */
+	omrsysinfo_get_processes, /*sysinfo_get_processes */


Missing space after /*.

keithc-ca · 2025-03-12T20:05:03Z

port/common/omrsysinfo.c

+uintptr_t
+omrsysinfo_get_processes(struct OMRPortLibrary *portLibrary, OMRProcessInfoCallback callback, void *userData)
+{
+        return OMRPORT_ERROR_NOT_SUPPORTED_ON_THIS_PLATFORM;


Please indent with a tab instead of spaces.

keithc-ca · 2025-03-12T20:08:13Z

port/unix/omrsysinfo.c

+#include <ctype.h>
+#include <stdint.h>


ctype.h is not needed (we can use just strtoull() without isdigit())

stdint.h is already included via inttypes.h

keithc-ca · 2025-03-12T20:09:37Z

port/unix/omrsysinfo.c

+omrsysinfo_get_processes(struct OMRPortLibrary *portLibrary, OMRProcessInfoCallback callback, void *userData)
+{
+#if defined(LINUX)
+	DIR *dir = NULL;


Suggest moving this declaration to the point of initialization (after line 7581):

DIR *dir = opendir("/proc");

keithc-ca · 2025-03-12T20:11:04Z

port/unix/omrsysinfo.c

+	char cmdline_path[PATH_MAX];
+	char comm_path[PATH_MAX];


Suggest we only need one of these buffers at a time (write the first path, try to read that file and only write the second path if necessary).

keithc-ca · 2025-03-12T20:24:58Z

port/unix/omrsysinfo.c

+			portLibrary->file_close(portLibrary, file);
+		}
+		/* If cmdline is empty, try reading from comm */
+		if (0 == bytes_read) {


This must check for negative values:

if (bytes_read <= 0) {

Likewise on line 7615.

keithc-ca · 2025-03-12T20:25:32Z

port/unix/omrsysinfo.c

+				command[i] = ' ';
+			}
+		}
+		/* Call the callback function with the process ID and command */


command should be NUL-terminated here:

command[bytes_read] = '\0';

keithc-ca · 2025-03-12T20:26:03Z

port/unix/omrsysinfo.c

+			}
+		}
+		/* Call the callback function with the process ID and command */
+		callback_result = callback((uintptr_t)strtoull(entry->d_name, NULL, 10), command, userData);


This should use pid computed above:

callback_result = callback(pid, command, userData);

port/unix/omrsysinfo.c

keithc-ca · 2025-03-12T20:27:45Z

port/unix/omrsysinfo.c

+#else
+	/* sysinfo_get_processes is not supported on this platform */
+	return OMRPORT_ERROR_SYSINFO_NOT_SUPPORTED;
+#endif /* LINUX */


Please correct this comment (and repeat it on line 7635):

#endif /* defined(LINUX) */

keithc-ca · 2025-03-14T16:15:33Z

port/common/omrport.tdf

@@ -914,6 +914,8 @@ TraceException=Trc_PRT_find_executable_name_invalidRead Group=sysinfo Overhead=1

 TraceException=Trc_PRT_sysinfo_get_tmp_failed_str_covert Group=sysinfo Overhead=1 Level=1 NoEnv Template="omrsysinfo_get_tmp: omrstr_convert() failed to convert unicode to modified utf8 with error code=%d"

+TraceException=Trc_PRT_sysinfo_get_processes_failedOpeningProcFS Group=sysinfo Overhead=1 Level=1 NoEnv Template="omrsysinfo_get_processes failed opening /proc = %d."


New tracepoints must be added to the end of the file; see https://github.com/eclipse-openj9/openj9/blob/master/doc/diagnostics/AddingTracepoints.md.

keithc-ca · 2025-03-14T16:25:16Z

port/unix/omrsysinfo.c

+omrsysinfo_get_processes(struct OMRPortLibrary *portLibrary, OMRProcessInfoCallback callback, void *userData)
+{
+#if defined(LINUX)
+	struct dirent *entry = NULL;


Locals should be declared in the smallest block possible; entry should be declared where it is initialized (on line 7588).
Only these need to be declared here:

uintptr_t callback_result = 0; DIR *dir = opendir("/proc");

keithc-ca · 2025-03-14T16:25:47Z

port/unix/omrsysinfo.c

+			continue;
+		}
+		/* Convert name to pid, skipping non-numeric entries. */
+		char *end = NULL;


It's not strictly legal in C to declare a local here.

keithc-ca · 2025-03-14T16:26:11Z

port/unix/omrsysinfo.c

+	if (NULL == dir) {
+		int32_t rc = findError(errno);
+		portLibrary->error_set_last_error(portLibrary, errno, rc);
+		Trc_PRT_sysinfo_get_processes_failedOpeningProcFS(rc); 


No trailing spaces, please (here and line 7629).

keithc-ca · 2025-03-14T16:27:44Z

port/unix/omrsysinfo.c

+		portLibrary->str_printf(portLibrary, path, sizeof(path), "/proc/%s/cmdline", entry->d_name);
+		file = portLibrary->file_open(portLibrary, path, EsOpenRead, 0);
+		if (0 != file) {
+			bytes_read = portLibrary->file_read(portLibrary, file, command, sizeof(command) - 1);


This may truncate commands which can be much longer than PATH_MAX (think megabytes).

keithc-ca · 2025-03-17T14:34:59Z

port/unix/omrsysinfo.c

+	#define ARG_MAX 2097152
+	char path[PATH_MAX];
+	char command[ARG_MAX];


This should not impose a fixed limit on the size of a command line. Greater than 4MB is unreasonable for a single stack frame.

keithc-ca · 2025-03-17T14:37:46Z

This should not make reference to JFR; yes, it might be one of the first consumers of this new API, but this project aims to be language neutral.

keithc-ca · 2025-03-19T20:52:34Z

port/unix/omrsysinfo.c

+		/* Try reading /proc/[pid]/cmdline. */
+		portLibrary->str_printf(portLibrary, path, sizeof(path), "/proc/%s/cmdline", entry->d_name);
+		file = portLibrary->file_open(portLibrary, path, EsOpenRead, 0);
+		if (0 != file) {


This test should be file >= 0 (a negative value signals failure, 0 is a valid file descriptor); same for line 7640.

keithc-ca · 2025-03-19T20:54:45Z

port/unix/omrsysinfo.c

+			for (;;) {
+				bytes_read = portLibrary->file_read(portLibrary, file, command, buffer_size - 1);
+				if (bytes_read < 0) {
+					break;
+				}
+				if (bytes_read == buffer_size - 1) {
+					/* Buffer may be too small, increase and retry. */
+					new_size = buffer_size * 2;
+					new_command = (char *)portLibrary->mem_reallocate_memory(
+							portLibrary,
+							command,
+							new_size, OMR_GET_CALLSITE(), OMRMEM_CATEGORY_PORT_LIBRARY);
+					if (NULL == new_command) {
+						break; /* Stop resizing if allocation fails. */
+					}
+					command = new_command;
+					buffer_size = new_size;
+					continue;
+				}
+				break;


This should be extracted into a new function which can be reused on line 7641.
Note that the second and subsequent reads should not write to the beginning of command.

I checked kernel header file and The result shows that TASK_COMM_LEN is defined as 16 in the kernel header file

278:#define TASK_COMM_LEN 16
1050: char comm[TASK_COMM_LEN];
1941: BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);
comm is a fixed-size buffer of 16 characters, so it doesn’t need resizing.

keithc-ca · 2025-03-19T20:55:21Z

port/unix/omrsysinfo.c

+		return (uintptr_t)(intptr_t)rc;
+	}
+	/* Allocate initial buffer. */
+	char *command = (char *)portLibrary->mem_allocate_memory(portLibrary, buffer_size, OMR_GET_CALLSITE(), OMRMEM_CATEGORY_PORT_LIBRARY);


This is not legal C code; variables must be declared at the beginning of a block.

keithc-ca · 2025-03-19T20:57:45Z

Please correct the spelling of "API" in the commit summary line and update the title here (also removing "jfr:").
Lines in the body of the commit message should be no more than 72 characters long.

Use /proc filesystem to extract command line. Used cmdline for user space processes and comm for others.

harship04 requested review from babsingh, mstoodle and 0xdaryl as code owners March 7, 2025 15:31

keithc-ca suggested changes Mar 7, 2025

View reviewed changes

harship04 force-pushed the systemprocess-x64 branch 2 times, most recently from e45d7db to ea48042 Compare March 11, 2025 11:23

keithc-ca suggested changes Mar 12, 2025

View reviewed changes

harship04 force-pushed the systemprocess-x64 branch 3 times, most recently from 8dcf87f to a947338 Compare March 14, 2025 14:37

keithc-ca suggested changes Mar 14, 2025

View reviewed changes

harship04 force-pushed the systemprocess-x64 branch 2 times, most recently from 62cef76 to 2626483 Compare March 14, 2025 20:17

keithc-ca suggested changes Mar 17, 2025

View reviewed changes

harship04 force-pushed the systemprocess-x64 branch 3 times, most recently from 498f43a to 0d65739 Compare March 19, 2025 10:09

keithc-ca suggested changes Mar 19, 2025

View reviewed changes

harship04 changed the title ~~jfr: Add an api for system processes~~ Add an API for system processes Mar 20, 2025

Add an API for system processes

ff79304

Use /proc filesystem to extract command line. Used cmdline for user space processes and comm for others.

harship04 force-pushed the systemprocess-x64 branch from 0d65739 to ff79304 Compare March 21, 2025 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an API for system processes #7681

Add an API for system processes #7681

harship04 commented Mar 7, 2025 •

edited

Loading

babsingh commented Mar 7, 2025

keithc-ca Mar 7, 2025

harship04 Mar 11, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 12, 2025

keithc-ca Mar 14, 2025

keithc-ca Mar 14, 2025

keithc-ca Mar 14, 2025

keithc-ca Mar 14, 2025

keithc-ca Mar 14, 2025

keithc-ca Mar 17, 2025

keithc-ca commented Mar 17, 2025

keithc-ca Mar 19, 2025

keithc-ca Mar 19, 2025

harship04 Mar 20, 2025

keithc-ca Mar 19, 2025

keithc-ca commented Mar 19, 2025

		@@ -914,6 +914,8 @@ TraceException=Trc_PRT_find_executable_name_invalidRead Group=sysinfo Overhead=1

		TraceException=Trc_PRT_sysinfo_get_tmp_failed_str_covert Group=sysinfo Overhead=1 Level=1 NoEnv Template="omrsysinfo_get_tmp: omrstr_convert() failed to convert unicode to modified utf8 with error code=%d"

		TraceException=Trc_PRT_sysinfo_get_processes_failedOpeningProcFS Group=sysinfo Overhead=1 Level=1 NoEnv Template="omrsysinfo_get_processes failed opening /proc = %d."

		#include <ctype.h>
		#include <stdint.h>

Add an API for system processes #7681

Are you sure you want to change the base?

Add an API for system processes #7681

Conversation

harship04 commented Mar 7, 2025 • edited Loading

babsingh commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca commented Mar 19, 2025

harship04 commented Mar 7, 2025 •

edited

Loading