On Nov 3, 2007 3:35 PM, Julian Foad <julianfoad@btopenworld.com> wrote:
> 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?
I committed the suggested fix in r28449 and opened issue 3045 to look
into the latter.
--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
Received on Thu Dec 13 00:43:48 2007