Bug [ 959379 ] Implicit close() should check for errors
I'd like to resolve bug 959379. Here's a naive fix. Comments? diff -u -r2.192 fileobject.c --- Objects/fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ Objects/fileobject.c 24 Oct 2004 20:58:49 -0000 @@ -304,7 +304,8 @@ PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + if ((*f->f_close)(f->f_fp) != 0) + perror("close failed"); Py_END_ALLOW_THREADS } PyMem_Free(f->f_setbuf); /Peter Åstrand <astrand@lysator.liu.se>
Peter Astrand <astrand@lysator.liu.se> writes:
I'd like to resolve bug 959379. Here's a naive fix. Comments?
If you're going to match the normal close() processing, shouldn't that check the result against EOF and not just != 0? If for example, the file object is being used for a pipe, the result of the close might be the result code of the child process which need not be an error. I think that only EOF should signify a failure of the close(), and only in that case is errno probably appropriate (which perror will pick up). -- David
On Mon, 25 Oct 2004, David Bolen wrote:
I'd like to resolve bug 959379. Here's a naive fix. Comments?
If you're going to match the normal close() processing, shouldn't that check the result against EOF and not just != 0? If for example, the
Yes. Thanks. Here's the new patch. OK to commit? diff -u -r2.192 fileobject.c --- fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ fileobject.c 26 Oct 2004 05:50:12 -0000 @@ -304,7 +304,8 @@ PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + if ((*f->f_close)(f->f_fp) == EOF) + perror("close failed"); Py_END_ALLOW_THREADS } PyMem_Free(f->f_setbuf); /Peter Åstrand <astrand@lysator.liu.se>
Peter Astrand wrote:
Yes. Thanks. Here's the new patch. OK to commit?
No. I feel that perror is inappropriate; PySys_WriteStderr might be slightly better. However, it might be that even raising an exception has some value. Regards, Martin
On Tue, 26 Oct 2004, "Martin v. Löwis" wrote:
Yes. Thanks. Here's the new patch. OK to commit?
No. I feel that perror is inappropriate; PySys_WriteStderr might be slightly better.
Here's a patch with PySys_WriteStderr: diff -u -r2.192 fileobject.c --- Objects/fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ Objects/fileobject.c 26 Oct 2004 18:44:19 -0000 @@ -300,12 +300,15 @@ static void file_dealloc(PyFileObject *f) { + int sts = 0; if (f->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + sts = (*f->f_close)(f->f_fp); Py_END_ALLOW_THREADS + if (sts == EOF) + PySys_WriteStderr("close failed: %s\n", strerror(errno)); } PyMem_Free(f->f_setbuf); Py_XDECREF(f->f_name);
However, it might be that even raising an exception has some value.
Is it really possible to raise an exception in response to something triggered by the GC? /Peter Åstrand <astrand@lysator.liu.se>
[Peter Astrand] ...
Is it really possible to raise an exception in response to something triggered by the GC?
It's possible but wholly useless. If you try, gc will detect that an exception was raised, and kill the Python process ungracefully, via Py_FatalError("unexpected exception during garbage collection"); You don't want to do that <wink>.
On Tue, 26 Oct 2004, "Martin v. Löwis" wrote:
Peter Astrand wrote:
+ PySys_WriteStderr("close failed: %s\n", strerror(errno));
This should use HAVE_STRERROR.
Like this?: diff -u -r2.192 fileobject.c --- dist/src/Objects/fileobject.c 11 Jun 2004 04:49:03 -0000 2.192 +++ dist/src/Objects/fileobject.c 7 Nov 2004 10:13:04 -0000 @@ -300,12 +300,19 @@ static void file_dealloc(PyFileObject *f) { + int sts = 0; if (f->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) f); if (f->f_fp != NULL && f->f_close != NULL) { Py_BEGIN_ALLOW_THREADS - (*f->f_close)(f->f_fp); + sts = (*f->f_close)(f->f_fp); Py_END_ALLOW_THREADS + if (sts == EOF) +#ifdef HAVE_STRERROR + PySys_WriteStderr("close failed: [Errno %d] %s\n", errno, strerror(errno)); +#else + PySys_WriteStderr("close failed: [Errno %d]\n", errno); +#endif } PyMem_Free(f->f_setbuf); Py_XDECREF(f->f_name); /Peter Åstrand <astrand@lysator.liu.se>
participants (4)
-
"Martin v. Löwis" -
David Bolen -
Peter Astrand -
Tim Peters