[Rd] [New Patch] Fix disk corruption when writing

Serguei Sokol sokol at insa-toulouse.fr
Fri Jul 7 15:54:05 CEST 2017 

Le 07/07/2017 à 01:09, Duncan Murdoch a écrit :
> On 06/07/2017 6:44 PM, Sokol Serguei wrote:
>> Duncan Murdoch has written at  Thu, 6 Jul 2017 13:58:10 -0400
>>> On 06/07/2017 5:21 AM, Serguei Sokol wrote:
>>>> I propose the following patch against the current
>>>> R-devel/src/main/connection.c (cf. attached file).
>>>> It gives (on my linux box):
>>>>  > fc=file("/dev/full", "w")
>>>>  > write.csv("a", file=fc)
>>>> Error in writeLines(paste(col.names, collapse = sep), file, sep = eol) :
>>>>    system call failure on writing
>>>>  > close(fc)
>>>>
>>>> Serguei.
>>>
>>> I suspect that flushing on every write will slow things down too much.
>> That's quite plausible.
>>
>>>
>>> I think a better approach is to catch the error in the Rconn_printf
>>> calls (as R-devel currently does), and also catch errors on
>>> con->close(con).  This one requires more changes to the source, so it
>>> may be a day or two before I commit.
>> I have testes it on file_close() and it works (cf. attached patch):
>>> fc=file("/dev/full", "w")
>>> write.csv("a", file=fc)
>>> close(fc)
>> Error in close.connection(fc) : closing file failed
>>
>>>
>>> One thing I have to look into:  is anyone making use of the fact that
>>> the R-level close.connection() function can return -1 to signal an
>>> error?  Base R ignores that, which is why we need to do something, but
>>> if packages are using it, things need to be changed carefully.  I
>>> can't just change it to raise an error instead.
>> As you can see in the patch, no need to change close.connection() function
>> but we have to add a test of con->status to all *_close() functions
>> (gzfile_close() and co.)
>
> You missed my point.  Currently the R close() function may return -1 to signal that there was an error closing.  We can't change that to an error if packages 
> are using it.
May be I missed it but finally, me too, I was saying that we don't have to do so.
Anyhow, the situation of writing to full disk cannot be passed in silence.
IMHO, trigger an error would be the most appropriate in this situation but if for legacy
or any other reason we cannot do so, let whistle a warning, at least.
Here few tests with a new small patch:
 > fc=file("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc))
[1] -1
Warning message:
In close.connection(fc) :
   closing '/dev/full' failed: No space left on device
 > fc=gzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc))
NULL
Warning message:
In close.connection(fc) :
   closing '/dev/full' failed: No space left on device
 > fc=xzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc))
NULL
Warning message:
In close.connection(fc) :
   closing '/dev/full' failed: No space left on device
 > fc=bzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc))
NULL
Warning message:
In close.connection(fc) :
   closing '/dev/full' failed: No space left on device

Note that if we test only status < 0 (without errno) then there are too many warnings
on seemingly "innocent" file closings.

Serguei.

>>>>
>>>> Le 05/07/2017 à 15:33, Serguei Sokol a écrit :
>>>>> Le 05/07/2017 à 14:46, Serguei Sokol a écrit :
>>>>>> Le 05/07/2017 à 13:09, Duncan Murdoch a écrit :
>>>>>>> On 05/07/2017 5:26 AM, January W. wrote:
>>>>>>>> I tried the newest patch, but it does not seem to work for me (on
>>>>>>>> Linux). Despite the check in Rconn_printf, the write.csv happily
>>>>>>>> writes
>>>>>>>> to /dev/full and does not report an error. When I added a
>>>>>>>> printf("%d\n",
>>>>>>>> res); to the Rconn_printf() definition, I see only positive values
>>>>>>>> returned by the vfprintf call.
>>>>>>>>
>>>>>>>
>>>>>>> That's likely because you aren't writing enough to actually
>>>>>>> trigger a write to disk during the write.  Writes are buffered,
>>>>>>> and the error doesn't happen
>>>>>>> until the buffer is written.
>>>>>> I can confirm this behavior with fvprintf(). Small and medium sized
>>>>>> writings
>>>>>> on /dev/full don't trigger error and 1MB does.
>>>>>>
>>>>>> But if fprintf() is used, it returns a negative value from the very
>>>>>> first byte written.
>>>>> I correct myself. In my test, fprintf() returned -1 for another
>>>>> reason (connection was already closed
>>>>> at this moment)
>>>>> However, if fvprintf(...) is followed by res=fflush(con) then res is -1
>>>>> if we try to write on /dev/full. May be we have to use this to trigger
>>>>> an error message in R.
>>>>>
>>>>> Serguei.
>>>>>
>>>>>>>   The regression test I put in had this problem; I'm working on
>>>>>>> MacOS and Windows, so I never got to actually try it before
>>>>>>> committing.
>>>>>>>
>>>>>>> Unfortunately, it doesn't look possible to catch the final flush
>>>>>>> of the buffer when the connection is closed, so small writes won't
>>>>>>> trigger any error.
>>>>>>>
>>>>>>> It's also possible that whatever system you're on doesn't signal
>>>>>>> an error when the write fails.
>>>>>>>
>>>>>>> Duncan Murdoch
>>>>>>>
>>>>>>>> Cheers,
>>>>>>>>
>>>>>>>> j.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4 July 2017 at 21:37, Duncan Murdoch <murdoch.duncan at gmail.com
>>>>>>>> <mailto:murdoch.duncan at gmail.com>> wrote:
>>>>>>>>
>>>>>>>>     On 04/07/2017 11:50 AM, Jean-Sébastien Bevilacqua wrote:
>>>>>>>>
>>>>>>>>         Hello,
>>>>>>>>         You can find here a patch to fix disk corruption.
>>>>>>>>         When your disk is full, the write function exit without
>>>>>>>> error
>>>>>>>>         but the file
>>>>>>>>         is truncated.
>>>>>>>>
>>>>>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17243
>>>>>>>> <https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17243>
>>>>>>>>
>>>>>>>>
>>>>>>>>     Thanks.  I didn't see that when it came through (or did and
>>>>>>>> forgot).
>>>>>>>>     I'll probably move the error check to a lower level (in the
>>>>>>>>     Rconn_printf function), if tests show that works.
>>>>>>>>
>>>>>>>>     Duncan Murdoch
>>>>>>>>
>>>>>>>>
>>>>>>>>     ______________________________________________
>>>>>>>>     R-devel at r-project.org <mailto:R-devel at r-project.org> mailing
>>>>>>>> list
>>>>>>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>> <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> -------- January Weiner --------------------------------------
>>>>>>>
>>>>>>> ______________________________________________
>>>>>>> R-devel at r-project.org mailing list
>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
>>
>>
>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: close1.patch
Type: text/x-patch
Size: 870 bytes
Desc: not available
URL: <https://stat.ethz.ch/pipermail/r-devel/attachments/20170707/3e17e20e/attachment.bin>

More information about the R-devel mailing list