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

Re: error-handling problem in FSFS?

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2007-11-04 00:35:31 CET

David Glasser wrote:
> There's the following function in fs_fs.c:
>
> svn_fs_fs__move_into_place(...)
> {
> /* Move the file into place. */
> err = svn_io_file_rename(old_filename, new_filename, pool);
> if (err && APR_STATUS_IS_EXDEV(err->apr_err))
> {
> ...
> }
>
> #ifdef __linux__
> {
> ...
> SVN_ERR(svn_io_file_open(&file, dirname, APR_READ, APR_OS_DEFAULT,
> pool));
> SVN_ERR(svn_io_file_flush_to_disk(file, pool));
> SVN_ERR(svn_io_file_close(file, pool));
> }
> #endif
>
> return err;
> }
>
> Let's say that there's an error returned from the rename at the top,
> but it's not EXDEV. And let's say that we're on Linux.
>
> Now, the error gets returned. But before it gets returned, the
> directory gets opened, flushed, and closed.
>
> This seems wrong to me; I think after the first block there should be
> an immediate "SVN_ERR(err)", and the end of the function should just
> be "return SVN_NO_ERROR". (Does that sound reasonable?)

Yes, that sounds reasonable.

> Now, assuming that this change is a good idea, the question is, can
> the current code actually have negative side effects? That is, can
> flushing the parent directory when there was some error in moving the
> file ever actually harm the repository? Or is it just an expensive
> no-op?

The directory operations might well fail for the same reason that the file
rename failed (maybe path not found, file system is read-only, ...), which
could result in the function returning with the directory opened, which could
be bad, but I feel almost anything like this that could happen with this wierd
code ordering could also happen without it.

Perhaps if we can't demonstrate a problem it's safer to wait till 1.5 just in
case it's actually serving some purpose the way it is.

But what got me interested enough to reply is:

   Should we move this OS-specific knowledge to svn_io (at least, or eventually
APR)? The "flush the directory entry for file F" part in particular, but pretty
much the whole move-file-into-place body of this function probably. Don't we
have something very similar (with not quite so strict flushing) in the WC code?

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Nov 4 00:36:17 2007

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