On Fri, 26 Jan 2024 17:29:10 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
>> 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>
>
> 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);
> }
Hello Gerard, my understanding is that the limit value configured may exceed
the int range. I wanted to avoid the overflow by casting it to int in such
cases. I had noticed close() takes an int, but I couldn't think of any other
way of avoiding the overflow at this place.
In the JVM parent process we do however limit it to INT_MAX. So maybe I should
assume that it will be an int cast it to int here, like you suggest, and add a
code comment explaining this? Does that sound OK?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1468270331
