Re: error-handling problem in FSFS?

[David Glasser <gl...@davidglasser.net>]

On Nov 3, 2007 3:35 PM, Julian Foad <ju...@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@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