RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]

[Jaikiran Pai]

On Wed, 31 Jan 2024 06:27:33 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE
>
> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 35:
> 
>> 33: #include "sys.h"
>> 34: #include "util.h"
>> 35: #include "error_messages.h"
> 
> Nit: to maintain include sort order this should have gone where `log_messages.h` was removed.

Hello David, I had actually first put it in that line you noted, but that then lead to a compilation error:


In file included from /jdk/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c:33:
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:35:20: error: unknown type name 'FILE'
void print_message(FILE *fp, const char *prefix,  const char *suffix,
                   ^
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:41:29: error: a parameter list without types is only allowed in a function definition
const char * jvmtiErrorText(jvmtiError);
                            ^
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:43:28: error: a parameter list without types is only allowed in a function definition
const char * jdwpErrorText(jdwpError);
                           ^
3 errors generated.



I think the reason for those compilation errors is that `error_messages.h` and a few other header files are not explicitly listing their necessary includes and are relying on transitive includes. So I decided to take the easy route out, by adding the `error_messages.h` after the `util.h` to rely on the transitive includes to get past that compilation error. 

I think to properly fix that compile error, this following change (which I tested locally and works) would be needed:


diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
index 92a9f457e8a..04f10bcb019 100644
--- a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
@@ -27,6 +27,7 @@
 #define JDWP_JDWP_H
 
 #include "JDWPCommands.h"
+#include "vm_interface.h"
 
 /*
  * JDWPCommands.h is the javah'ed version of all the constants defined
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
index 4126b76f226..810ab384f1a 100644
--- a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
@@ -26,6 +26,9 @@
 #ifndef JDWP_ERROR_MESSAGES_H
 #define JDWP_ERROR_MESSAGES_H
 
+#include <stdio.h>
+#include "JDWP.h"
+
 /* It is assumed that ALL strings are UTF-8 safe on entry */
 #define TTY_MESSAGE(args) ( tty_message args )
 #define ERROR_MESSAGE(args) ( \
diff --git a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
index 44f8a463522..0c6ee5b8b6a 100644
--- a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
+++ b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
@@ -30,9 +30,9 @@
 #include <unistd.h>
 #include <string.h>
 #include <ctype.h>
+#include "error_messages.h"
 #include "sys.h"
 #include "util.h"
-#include "error_messages.h"
 
 static char *skipWhitespace(char *p) {
     while ((*p != '\0') && isspace(*p)) {



Do you think this should be fixed? I can file a JBS issue for it and address it as a separate PR.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472425903