RFR [8055421]: (fs) bad error handling in java.base/unix/native/libnio/fs/UnixNativeDispatcher.c

[Martin Buchholz]

On Sun, Aug 24, 2014 at 1:54 PM, Ivan Gerasimov <ivan.gerasimov at oracle.com>
wrote:

> Thank you Alan for the input!
>
> I've updated the webrev as you suggested:
> http://cr.openjdk.java.net/~igerasim/8055421/2/webrev/
>
>
Ivan, this change looks good!

(if UnixNativeDispatcher.fclose is only designed for reading, not writing,
there should be at the very least a comment.  It's natural for future
readers to assume this is a general wrapper for fclose of either kind of
stream.)

With the wrapper for close we currently ignore any error that might have
> happened.
> Don't we want to revise it with this change?
>
>
I agree that we should add error checking for close as you suggest, but
let's do that in another change.


> For example, note the caution on the Linux man page:
> Not  checking  the  return  value of close() is a common but nevertheless
> serious programming error.  It is quite possible that errors on a previous
> write(2) operation are first reported at the final close().  Not checking
> the return value when closing the file may lead to silent loss of data.
>
>
> Sincerely yours,
> Ivan
>
>
> On 24.08.2014 17:13, Alan Bateman wrote:
>
>> On 22/08/2014 21:59, Ivan Gerasimov wrote:
>>
>>> How about another approach?
>>> If we face EINTR, we can try to fallback to restartable close():
>>>
>> I was away for a few days and just catching up on this thread now.
>>
>> I agree with Florian's original comment as there is something fishy about
>> the original bug report and it would be useful to know if there is really a
>> case where we are calling this native method with errno set. In any case,
>> the long standing rule with close is that you never retry it after EINTR
>> because the behavior is undefined. It's an oversight in the original
>> implementation to have used a restart loop in fclose and to have used the
>> RESTARTABLE macro in closedir. Apologies about those. The proposed change
>> to dup looks okay, that would be a problem if dup were called when there
>> aren't any file descriptors available.
>>
>> On fclose then one thing to note is that it is only used on Solaris when
>> reading /etc/mnttab, I don't think it is used elsewhere. So write buffering
>> isn't an issue with the current usage. If there was writing then Martin's
>> suggestion to use a restartable fflush would be good to do. It also means
>> that having fclose ignore EINTR as per your 1/webrev is okay too. For
>> completeness then we could add a new method fflush method that might get
>> used someday.
>>
>> For closedir then dropping the use of RESTARTABLE and ignoring EINTR
>> should be okay.
>>
>> -Alan.
>>
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20140824/d92f2e54/attachment-0001.html>