error-handling problem in FSFS?

There's the following function in fs_fs.c:

svn_error_t *
svn_fs_fs__move_into_place(const char *old_filename,
                           const char *new_filename,
                           const char *perms_reference,
                           apr_pool_t *pool)
{
  svn_error_t *err;

  SVN_ERR(svn_fs_fs__dup_perms(old_filename, perms_reference, pool));

  /* Move the file into place. */
  err = svn_io_file_rename(old_filename, new_filename, pool);
  if (err && APR_STATUS_IS_EXDEV(err->apr_err))
    {
      apr_file_t *file;

      /* Can't rename across devices; fall back to copying. */
      svn_error_clear(err);
      SVN_ERR(svn_io_copy_file(old_filename, new_filename, TRUE, pool));

      /* Flush the target of the copy to disk. */
      SVN_ERR(svn_io_file_open(&file, new_filename, APR_READ,
                               APR_OS_DEFAULT, pool));
      SVN_ERR(svn_io_file_flush_to_disk(file, pool));
      SVN_ERR(svn_io_file_close(file, pool));
    }

#ifdef __linux__
  {
    /* Linux has the unusual feature that fsync() on a file is not
       enough to ensure that a file's directory entries have been
       flushed to disk; you have to fsync the directory as well.
       On other operating systems, we'd only be asking for trouble
       by trying to open and fsync a directory. */
    const char *dirname;
    apr_file_t *file;

    dirname = svn_path_dirname(new_filename, pool);
    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?)

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?

--dave

-- 
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org