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

[Martin Buchholz]

Looks good!

On Thu, Dec 17, 2015 at 12:14 AM, vyom <vyom.tewari at 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.tewari at 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.Riggs at 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>