RFR: 8303942: os::write should write completely [v4]

David Holmes dholmes at openjdk.org
Mon May 8 01:53:27 UTC 2023


On Sun, 7 May 2023 09:42:28 GMT, Afshin Zafari <duke at openjdk.org> wrote:

>> `os::write` is implemented using loops until the whole bytes are written. All uses of `os::write` in a loop are changed to single call. 
>> Platform dependent versions of the `os::write` are also renamed and moved to private sections accordingly.
>> Wrong uses/interpretations of return values from `os::write` in JFR code are corrected.
>> 
>> ###Test
>> local: hotspot tier1
>> mach5: tiers 1-5
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8303942: os::write should write completely

Changes requested by dholmes (Reviewer).

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?

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?

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.

-------------

PR Review: https://git.openjdk.org/jdk/pull/13750#pullrequestreview-1416020946
PR Review Comment: https://git.openjdk.org/jdk/pull/13750#discussion_r1186958631
PR Review Comment: https://git.openjdk.org/jdk/pull/13750#discussion_r1186959907
PR Review Comment: https://git.openjdk.org/jdk/pull/13750#discussion_r1186960569


More information about the hotspot-dev mailing list