Re: RFR 4823133: RandomAccessFile.length() is not thread-safe

[Daniel Fuchs]

Hu Vyom,

On 17/12/15 17:58, Martin Buchholz wrote:

Looks good!

Since you got Martin's blessing I will sponsor this
and push it for you :-)

best regards,

-- daniel

On Thu, Dec 17, 2015 at 12:14 AM, vyom <vyom.tew...@oracle.com> wrote:

Hi Martin,

thanks for review, i undated the
webrev(http://cr.openjdk.java.net/~vtewari/4823133/webrev0.2/
<http://cr.openjdk.java.net/%7Evtewari/4823133/webrev0.2/>) as per your
comment after confirming that the corresponding "fd" if opened with
"open64".

Thanks,
Vyom



On Thursday 17 December 2015 12:38 AM, Martin Buchholz wrote:

Calling naked fstat without _FILE_OFFSET_BITS=64 is a bug.  Don't you
need to call fstat64 if it's available?

+jlong
+handleGetLength(FD fd)
+{
+    struct stat sb;
+    if (fstat(fd, &sb) == 0) {
+        return sb.st_size;
+    } else {
+        return -1;
+    }
+}

On Wed, Dec 16, 2015 at 5:02 AM, vyom <vyom.tew...@oracle.com> wrote:

Hi,

Updated the webrev(http://cr.openjdk.java.net/~vtewari/4823133/webrev0.1/
<http://cr.openjdk.java.net/%7Evtewari/4823133/webrev0.1/>) as per Martin
review comment removed the _FILE_OFFSET_BITS from source.

Thanks,
Vyom



On Tuesday 15 December 2015 10:55 PM, Martin Buchholz wrote:

_FILE_OFFSET_BITS is generally an all-or-nothing thing, because it
affects interoperability between translation units.  It would be good
to convert all of the JDK build to use -D_FILE_OFFSET_BITS=64, but
that would be a big job.  So traditionally the JDK has instead used
the functions made available via _LARGEFILE64_SOURCE.  But that is
also a JDK-wide job now, because every call to plain stat in the
source code is broken on 32-bit systems with 64-bit inodes, which are
becoming more common.

I recommend the _FILE_OFFSET_BITS=64 strategy, but it's probably a job
for build-dev, not core-libs-dev.




On Tue, Dec 15, 2015 at 8:31 AM, Roger Riggs <roger.ri...@oracle.com>
wrote:

Hi Yvom,

Minor comments:

src/java.base/share/native/libjava/RandomAccessFile.c:
    - "length fail" might be clearer as "GetLength failed"

src/java.base/unix/native/libjava/io_util_md.c:

    - Please add a comment before the define of FILE_OFFSET_BITS to
indicate
where it is used and why it is there.
    - BTW, are there any unintended side effects?
      Perhaps a different issue but perhaps 64 bit offsets should be
used
everywhere

src/java.base/windows/native/libjava/io_util_md.c
    - Line 592: Using INVALID_HANDLE_VALUE is better than -1 and is used
elsewhere in the file
      BTW, Testing for invalid handle might be unnecessary since the
call
to
GetFileSizeEx will fail
      if it is invalid, yielding the same result.

Roger


On 12/10/2015 5:52 AM, vyom wrote:

Hi All,

Please review my changes for below bug.

Bug: JDK-4823133 : RandomAccessFile.length() is not thread-safe

Webrev:http://cr.openjdk.java.net/~vtewari/4823133/webrev0.0/
<http://cr.openjdk.java.net/%7Evtewari/4823133/webrev0.0/>

This change ensure that  length() does not temporarily changes the
file
pointer and it will make sure that there is no race
condition in case of multi thread uses.

Thanks,
Vyom