On Wed, 6 Aug 2025 10:55:07 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>> Guanqiang Han has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - a small fix >> - Merge remote-tracking branch 'upstream/master' into 8364312 >> - Update exec_md.c >> >> correct an compilation error >> - 8364312: debug agent should set FD_CLOEXEC flag rather than explicitly >> closing every open file >> >> Create a new function that marks all file descriptors found in >> /proc/self/fd with the FD_CLOEXEC flag to ensure they are automatically >> closed upon execution of a new program via exec(). > > Hi - > > (The code duplication from src/java.base/unix/native/libjava/childproc.c is > unfortunate but maybe it's hard to avoid.) > > exec_md.c has the existing closeDescriptors() that "returns 0 on failure and > 1 on success." which is unusual and unfortunate as zero is a typical success > value. > > As this is only used locally, this should be an opportunity to change that, > and have the new markDescriptorsCloseOnExec() return more normal values, like > 0 for success and -1 for failure (like in childproc.c which this borrows > from). > (Unless Chris disagrees or sees wider usage...) > > > Another existing problem: > forkedChildProcess() comments that it will: "Close all file descriptors that > have been copied over" > But it calls closeDescriptors() which says it: "Closes every file descriptor > that is listed as a directory" > > Can we make the comments consistent while we are here? 8-) > > The comments before forkedChildProcess() at line 118 and within it at 124 > talk about actual closing, not marking as close on exec. Hi @kevinjwalls, thank you for the detailed feedback — much appreciated! I agree with your suggestions and have updated the PR accordingly. Please take another look when you have a moment. ------------- PR Comment: https://git.openjdk.org/jdk/pull/26568#issuecomment-3159927768
