RFR(M): 8183227: read/write APIs in class os shall return ssize_t

[Thomas Stüfe]

Hi Christoph,

good change so far.

src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c:  ok
src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c: ok
src/os/aix/vm/os_aix.cpp: ok
src/os/aix/vm/os_aix.inline.hpp: ok
src/os/bsd/vm/os_bsd.cpp: ok
src/os/bsd/vm/os_bsd.inline.hpp: ok
src/os/linux/vm/os_linux.cpp: ok
src/os/linux/vm/os_linux.inline.hpp: ok
src/os/solaris/vm/os_solaris.cpp: ok
src/os/windows/vm/os_windows.cpp: ok
src/os/windows/vm/os_windows.inline.hpp: ok
src/share/vm/classfile/classLoader.cpp: ok
src/share/vm/classfile/sharedPathsMiscInfo.hpp: ok

src/share/vm/compiler/compileLog.cpp:

-      size_t nr; // number read into buf from partial log
+      ssize_t nr; // number read into buf from partial log
       // Copy data up to the end of the last <event> element:
       julong to_read = log->_file_end;
       while (to_read > 0) {
         if (to_read < (julong)buflen)
-              nr = (size_t)to_read;
+              nr = (ssize_t)to_read;
         else  nr = buflen;
         nr = read(partial_fd, buf, (int)nr);
         if (nr <= 0)  break;
         to_read -= (julong)nr;
-        file->write(buf, nr);
+        file->write(buf, (size_t)nr);
       }

nr is used both as input and output variable. Could you disentangle this by
providing two variables, that would make the cast unnecessary. Even cleaner
would be to make the len/positon arguments for read/pread a size_t.

src/share/vm/compiler/directivesParser.cpp: ok
src/share/vm/memory/filemap.cpp: ok
src/share/vm/runtime/os.hpp: ok

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

General remarks (up to you if you fix them in this issue or not):

- Please leave whitespace only changes out, makes it difficult to read the
change
- The length and, for pread(3), position arguments are size_t. To avoid
casts, it would make sense to make the corresponding arguments for
os::read/os::read_at etc size_t too.
- os::read_at() and os::restartable_read() are not used anywhere and can be
deleted.
- A lot of those IO functions (e.g. os::write()) live in xx.inline.hpp
header files. That is unncessary, they should live in xxx.cpp files.


Kind Regards, Thomas


On Mon, Jul 3, 2017 at 9:36 AM, Langer, Christoph <christoph.langer at sap.com>
wrote:

> Ping: Any comments on this change...?
>
> From: Langer, Christoph
> Sent: Donnerstag, 29. Juni 2017 12:50
> To: hotspot-runtime-dev at openjdk.java.net
> Subject: RFR(M): 8183227: read/write APIs in class os shall return ssize_t
>
> Hi folks,
>
> please review the following change:
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8183227
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8183227.0/
>
> When looking at the read() and write() APIs and its siblings on the
> platforms, you find the return value mostly being of signed type ssize_t
> vs. the current unsigned return type size_t of the methods in class os.
> With my proposed patch I try to address that and change the signatures of
> these methods to ssize_t.
>
> At the places where the methods were called so far, I could find 2 places
> where usage of the wrong type could be critical:
> src/share/vm/compiler/compileLog.cpp (where actually ::read is used) and
> src/share/vm/compiler/directivesParser.cpp where a wrong array access
> could happen
>
> As for the class os, I'm wondering if the methods "read_at" and
> "restartable_read" are still required? My source code scan tells me that
> there are no users of them. Shall I maybe remove them completely from the
> code?
>
> Thanks & Best regards
> Christoph
>
>
>