RFR [XS] : 8205416 : windows: fix checking of CloseHandle return code in Java_java_io_FileCleanable_cleanupClose0

[Thomas Stüfe]

Looks good, Matthias.

..Thomas

On Wed, Jun 20, 2018 at 2:39 PM, Baesken, Matthias
<matthias.baesken at sap.com> wrote:
> Hi Thomas and Alan, thanks for the reviews.
>
> I adjusted the expression  like Thomas suggested and adjusted the file copyright year too :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205416.1/
>
>
> Best regards, Matthias
>
>
>
>> -----Original Message-----
>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>> Sent: Mittwoch, 20. Juni 2018 14:17
>> To: Baesken, Matthias <matthias.baesken at sap.com>
>> Cc: core-libs-dev at openjdk.java.net
>> Subject: Re: RFR [XS] : 8205416 : windows: fix checking of CloseHandle return
>> code in Java_java_io_FileCleanable_cleanupClose0
>>
>> Good catch.
>>
>> Could you please change the expression to either
>>
>> CloseHandle() == FALSE (uppercase)
>>
>> or
>>
>> !CloseHandle().
>>
>> ? which would be the standard windows way of writing this.
>>
>> I am curious whether we now get a bunch of follow up errors, since
>> before we never catched a failing CloseHandle. A typical reason why
>> this could fail would be a double close.
>>
>> ..Thomas
>>
>>
>> On Wed, Jun 20, 2018 at 2:09 PM, Baesken, Matthias
>> <matthias.baesken at sap.com> wrote:
>> > Please review this small fix for a return code handling of windows function
>> CloseHandle .
>> >
>> > MSDN documents  CloseHandle here :   https://msdn.microsoft.com/de-
>> de/library/windows/desktop/ms724211(v=vs.85).aspx
>> > ....
>> >    Return value
>> >    If the function succeeds, the return value is nonzero.
>> >    If the function fails, the return value is zero. To get extended error
>> information, call GetLastError<https://msdn.microsoft.com/de-
>> de/library/windows/desktop/ms679360(v=vs.85).aspx>.
>> >
>> >
>> > However  until this patch,  Java_java_io_FileCleanable_cleanupClose0
>> > Checked for return code   -1  of  CloseHandle   in error cases .
>> >
>> >
>> >
>> >
>> > Bug:
>> >
>> > https://bugs.openjdk.java.net/browse/JDK-8205416
>> >
>> >
>> > Webrev :
>> >
>> > http://cr.openjdk.java.net/~mbaesken/webrevs/8205416/
>> >
>> >
>> >
>> > Thanks, Matthias