On Fri, 26 Jan 2024 17:33:39 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
> Have you considered using `fcntl(d, F_SETFD, 1)` instead of the fancy > `closeDescriptors()`? > > I haven't tested it myself, but per the `man close` 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; > ``` I hadn't considered that. Now that you mentioned it, I had a look at what `man close` says and `man fcntl` says. My understanding of it is that we use `fcntl(fd, F_SETFD, FD_CLOEXEC)` (the `1` in the example corresponds to `FD_CLOEXEC`) if we want to close a specific file descriptor only if a subsequent call to `execvp` (or similar exec function) succeeds. If the subsequent call to `execvp` fails, then the file descriptor won't be closed. From what I understand of the code in jdwp, I don't think we need that behaviour here. Plus using `fcntl` won't get rid of our `closeDescriptors()` function. We will still need it, it's just the call to `close()` within the loop that gets replaced by a call to `fcntl`. > Should this be: > (void)close(fd); > and > (void)closedir(dp); > to show that we're ignoring the return value? Done. I've updated the PR to cast these calls to void. ------------- PR Comment: https://git.openjdk.org/jdk/pull/17588#issuecomment-1913151651 PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1468462470
