On Fri, 26 Jan 2024 15:57:57 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=<cmd>` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > use the right include for rlim_t - <sys/resource.h> Have you considered using `fcntl(d, F_SETFD, 1)` instead of the fancy `closeDescriptors()`? I haven't tested it myself, but per the man page: Most of the descriptors can be rearranged with dup2(2) or deleted with close() before the execve is attempted, but if some of these descriptors will still be needed if the execve fails, it is necessary to arrange for them to be closed if the execve succeeds. For this reason, the call “fcntl(d, F_SETFD, 1)” is provided, which arranges that a descriptor will be closed after a successful execve; src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 129: > 127: * and assume all were opened for the parent process and > 128: * copied over to this child process. we close them all */ > 129: const rlim_t max_fd = sysconf(_SC_OPEN_MAX); Why not use `int`, like the original code, instead of `rlim_t` - as per man page `close()` expects `int`, not `rlim_t`, ex: const int max_fd = (int)sysconf(_SC_OPEN_MAX); JDI_ASSERT(max_fd != -1); int fd; /* leave out standard input/output/error file descriptors */ for (fd = STDERR_FILENO + 1; fd < max_fd; fd++) { (void)close(fd); } ------------- Changes requested by gziemski (Committer). PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1846188473 PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1467927052
