RFR: 8303942: os::write should write completely [v4]
Afshin Zafari
duke at openjdk.org
Mon May 8 12:12:25 UTC 2023
On Mon, 8 May 2023 01:41:27 GMT, David Holmes <dholmes at openjdk.org> wrote:
>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>>
>> 8303942: os::write should write completely
>
> src/hotspot/share/jfr/recorder/repository/jfrEmergencyDump.cpp line 378:
>
>> 376: assert(size > 0, "invariant");
>> 377: unsigned int bytes_read = 0;
>> 378: unsigned int bytes_written = 0;
>
> Why have you changed this to a 32-bit type?
Changed back to `int64_t`.
> src/hotspot/share/runtime/os.hpp line 232:
>
>> 230: static void get_summary_cpu_info(char* buf, size_t buflen);
>> 231: static void get_summary_os_info(char* buf, size_t buflen);
>> 232: static ssize_t pd_write(int fd, const void *buf, size_t nBytes);
>
> What is the required meaning of the return value here? I'm assuming < 0 -> error; while >= 0 -> bytes written?
Comment added.
> src/hotspot/share/runtime/os.hpp line 649:
>
>> 647: static ssize_t read_at(int fd, void *buf, unsigned int nBytes, jlong offset);
>> 648: // Writes the bytes completely. Returns 0 on success, -1 otherwise.
>> 649: static ssize_t write(int fd, const void *buf, size_t nBytes);
>
> We don't need a ssize_t return type if we only return -1 or 0. Also this should be specified to return OS_ERR on error and OS_OK on success. The callers should also check for OS_ERR and OS_OK rather than -1, < 0, > 0 etc.
>
> Or we could simply make this a boolean function.
Return value changed to `bool`. All calls changed accordingly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13750#discussion_r1187366331
PR Review Comment: https://git.openjdk.org/jdk/pull/13750#discussion_r1187367072
PR Review Comment: https://git.openjdk.org/jdk/pull/13750#discussion_r1187367995
More information about the hotspot-dev
mailing list