Showstopper bug in FS format file code

[Greg Hudson <gh...@MIT.EDU>]

In r13387 Karl added the following to svn_fs_fs__open:

  /* Read the FS format number. */
  err = svn_io_read_version_file (&format, path_format (fs, pool), pool);
  if (err && APR_STATUS_IS_ENOENT (err->apr_err))
    {
      /* Pre-1.2 filesystems did not have a format file (you could say
         they were format "0"), so they get upgraded on the fly. */
      svn_error_clear (err), err = NULL;
      format = SVN_FS_FS__FORMAT_NUMBER;
      SVN_ERR (svn_io_write_version_file
               (path_format (fs, pool), format, pool));
    }
  else if (err)
    return err;

This has three problems:

  * We might not have write access to the repository, in which case we
    will error out inappropriately.

  * We might have a umask which would prevent other users from reading
    the new format file, which would cause them to error out
    inappropriately when they try to access it.

  * Philosophically, read-only operations on an FSFS repository should
    leave no trace, so even if we opportunistically try to create the
    FS format file and use appropriate permissions, we would be
    tarnishing the design.

We can't release 1.2 with these problems.

For the moment, I suggest treating an empty format file as being equal
to an FS format of 1, without creating the format file.  (If we need
to actually bump the FS format in the future, we'll get into the same
big argument about auto-upgrading as we did for the repository format
file previously.)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Showstopper bug in FS format file code

["C. Michael Pilato" <cm...@collab.net>]

Greg Hudson <gh...@MIT.EDU> writes:

> For the moment, I suggest treating an empty format file as being equal
> to an FS format of 1, without creating the format file.  (If we need
> to actually bump the FS format in the future, we'll get into the same
> big argument about auto-upgrading as we did for the repository format
> file previously.)

Works for me.  Though, I wonder if we couldn't just make FSFS commits
do the "if no format file, create one" step.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Showstopper bug in FS format file code

[kf...@collab.net]

Greg Hudson <gh...@MIT.EDU> writes:
> In r13387 Karl added the following to svn_fs_fs__open:
> 
>   /* Read the FS format number. */
>   err = svn_io_read_version_file (&format, path_format (fs, pool), pool);
>   if (err && APR_STATUS_IS_ENOENT (err->apr_err))
>     {
>       /* Pre-1.2 filesystems did not have a format file (you could say
>          they were format "0"), so they get upgraded on the fly. */
>       svn_error_clear (err), err = NULL;
>       format = SVN_FS_FS__FORMAT_NUMBER;
>       SVN_ERR (svn_io_write_version_file
>                (path_format (fs, pool), format, pool));
>     }
>   else if (err)
>     return err;
> 
> This has three problems:
> 
>   * We might not have write access to the repository, in which case we
>     will error out inappropriately.
> 
>   * We might have a umask which would prevent other users from reading
>     the new format file, which would cause them to error out
>     inappropriately when they try to access it.
> 
>   * Philosophically, read-only operations on an FSFS repository should
>     leave no trace, so even if we opportunistically try to create the
>     FS format file and use appropriate permissions, we would be
>     tarnishing the design.
> 
> We can't release 1.2 with these problems.
> 
> For the moment, I suggest treating an empty format file as being equal
> to an FS format of 1, without creating the format file.  (If we need
> to actually bump the FS format in the future, we'll get into the same
> big argument about auto-upgrading as we did for the repository format
> file previously.)

Thanks for catching this, Greg.  I'll fix it on trunk, in the way you
suggest above, and propose the fix for backport to 1.2.x.

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org