[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

missing close of rev file in fsfs - get_dir_contents?

From: Johan Corveleyn <jcorvel_at_gmail.com>
Date: Wed, 24 Feb 2010 02:38:51 +0100

Hi devs,

I think I may have found a bug in fs_fs.c, while I was
examining/profiling the code. However, I am completely new at
subversion development, and quite unexperienced in c (I'm a java
developer actually, with some notions of c, but trying to learn fast),
so I may be imagining things. If so, feel free to let me know I
completely misunderstood things :-).

I think the rev files that are opened inside get_dir_contents (called
by svn_fs_fs__get_dir_contents) are not closed. At least I can't see
them being closed. I have instrumented the code with printf statements
(particularly inside svn_io_file_open, svn_io_file_close and in
apr_file_open, apr_file_close) to see what's happening. For the open
that happens inside get_dir_contents, I don't see svn_io_file_close
nor apr_file_close being hit later on.

Looking at the source code I see:
- get_dir_contents calls read_representation
- read_representation calls rep_read_get_baton, which creates a
rep_read_baton and opens the rev file inside build_rep_list ->
create_rep_state -> create_rep_state_body
- read_representation wraps it in a stream, and sets the close
function to rep_read_contents_close
- get_dir_contents reads stuff and closes the stream

Now, the close callback of the stream is rep_read_contents_close, which is:
static svn_error_t *
rep_read_contents_close(void *baton)
  struct rep_read_baton *rb = baton;


  return SVN_NO_ERROR;

Apparently, this does not close the file. After this callback is run,
neither svn_io_file_close nor apr_file_close are being executed.

I read in the Community Guide (section "Exception handling") that "All
files opened with apr_file_open are closed at pool cleanup". If that's
correct, this closing either doesn't happen through apr_file_close, or
the above code doesn't destroy (cleanup?) the correct pools or the
filehandle is not associated with this pool or something.

If this is a bug, it's probably read_representation's fault (not
installing a good close callback on the stream). That function is also
used from other places besides get_dir_contents, so they may be
affected as well ...

This was tested with trunk of today (2010-02-23), on Windows (yes, I
actually managed to build on Windows :-)).


P.S.: I know littering the code with printf's is a quite primitive way
of debugging/profiling. But hey, I already have to learn c, apr and
the subversion source code, not to mention reading HACKING and the
subversion design doc, so give me a break :-).
P.P.S.: I'm actually looking at this code to see if I can do something
to improve fsfs' I/O behavior, opening and closing the same files all
the time (really, really costly if your repo is on a NAS mounted via
NFS). Maybe this is a hopeless or stupid mission, but I'm trying
anyway :-).
Received on 2010-02-24 02:39:25 CET

This is an archived mail posted to the Subversion Dev mailing list.