Tempfile cleanup sloppiness in blame.c

[Greg Hudson <gh...@MIT.EDU>]

In libsvn_client/blame.c's cleanup_tempfile() we have:

  /* the file may or may not have been closed; try it */
  apr_file_close (file);

If we close a file which we have already closed, the result is a
close(-1), which is not valgrind-clean.  Is there any way we can be
more precise?  I see no guarantee in apr_file_io.h that it is okay to
close a file twice, and normally it's not kosher to run an object's
destructor more than once.  (It would seem like a valid implementation
of the apr_file_io.h interface to allocate each file in a subpool and
destroy the subpool in apr_file_close(), in which case it would
definitely not work right to close files more than once.)

Alternatively, we could modify APR to check if file->filedes is -1
before closing, and document that it's okay to close files twice.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Tempfile cleanup sloppiness in blame.c

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Sun, 12 Sep 2004, Greg Hudson wrote:

> In libsvn_client/blame.c's cleanup_tempfile() we have:
>
>   /* the file may or may not have been closed; try it */
>   apr_file_close (file);
>
> If we close a file which we have already closed, the result is a
> close(-1), which is not valgrind-clean.  Is there any way we can be
> more precise?  I see no guarantee in apr_file_io.h that it is okay to
> close a file twice, and normally it's not kosher to run an object's
> destructor more than once.  (It would seem like a valid implementation
> of the apr_file_io.h interface to allocate each file in a subpool and
> destroy the subpool in apr_file_close(), in which case it would
> definitely not work right to close files more than once.)
>
I see the problem, but consider mroe theoretical than practical. I think
it has been in blame for a long time (it was there before I started
messing that code up last spring at least).

I may look at it sometimes if no one else beats me. Maybe you should file
an issue.

BTW, the reason for closing the file "in case" is that it has to be closed
before deleted and the cleanups will do it in reverse order. This code
path is used if an error occurs.

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org