You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2014/10/07 12:41:14 UTC
svn commit: r1629854 - in /subversion/trunk/subversion/libsvn_fs_fs: fs.c
fs_fs.c
Author: stefan2
Date: Tue Oct 7 10:41:14 2014
New Revision: 1629854
URL: http://svn.apache.org/r1629854
Log:
In FSFS, always use the same function to read the 'current' file.
Apart from the consistency aspect, this no longer lets atoi() mask
'current' file corruptions. Recovery must be adopted to this.
* subversion/libsvn_fs_fs/fs_fs.c
(get_youngest): Change signature because we will need the format
info as well - not just a repo path. Call the util
function to read and interpret the 'current' file.
(with_some_lock_file,
svn_fs_fs__open,
svn_fs_fs__youngest_rev,
svn_fs_fs__ensure_revision_exists): Update callers.
* subversion/libsvn_fs_fs/fs.c
(fs_open_for_recovery): Make sure, a valid 'current' file is available
for repository recovery.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/fs.c
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs.c?rev=1629854&r1=1629853&r2=1629854&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Tue Oct 7 10:41:14 2014
@@ -44,6 +44,7 @@
#include "rep-cache.h"
#include "revprops.h"
#include "transaction.h"
+#include "util.h"
#include "verify.h"
#include "svn_private_config.h"
#include "private/svn_fs_util.h"
@@ -348,20 +349,47 @@ fs_open_for_recovery(svn_fs_t *fs,
apr_pool_t *pool,
apr_pool_t *common_pool)
{
+ svn_error_t * err;
+ svn_revnum_t youngest_rev;
+ apr_pool_t * subpool = svn_pool_create(pool);
+
/* Recovery for FSFS is currently limited to recreating the 'current'
file from the latest revision. */
/* The only thing we have to watch out for is that the 'current' file
- might not exist. So we'll try to create it here unconditionally,
- and just ignore any errors that might indicate that it's already
- present. (We'll need it to exist later anyway as a source for the
- new file's permissions). */
+ might not exist or contain garbage. So we'll try to read it here
+ and provide or replace the existing file if we couldn't read it.
+ (We'll also need it to exist later anyway as a source for the new
+ file's permissions). */
- /* Use a partly-filled fs pointer first to create 'current'. This will fail
- if 'current' already exists, but we don't care about that. */
+ /* Use a partly-filled fs pointer first to create 'current'. */
fs->path = apr_pstrdup(fs->pool, path);
- svn_error_clear(svn_io_file_create(svn_fs_fs__path_current(fs, pool),
- "0 1 1\n", pool));
+
+ SVN_ERR(initialize_fs_struct(fs));
+
+ /* Figure out the repo format and check that we can even handle it. */
+ SVN_ERR(svn_fs_fs__read_format_file(fs, subpool));
+
+ /* Now, read 'current' and try to patch it if necessary. */
+ err = svn_fs_fs__youngest_rev(&youngest_rev, fs, subpool);
+ if (err)
+ {
+ const char *file_path;
+
+ /* 'current' file is missing or contains garbage.
+ * Start with HEAD = 0 in that case. */
+ svn_error_clear(err);
+ file_path = svn_fs_fs__path_current(fs, subpool);
+
+ /* Best effort to ensure the file exists and is valid.
+ * This may fail for r/o filesystems etc. */
+ SVN_ERR(svn_io_remove_file2(file_path, TRUE, subpool));
+ SVN_ERR(svn_io_file_create_empty(file_path, subpool));
+ SVN_ERR(svn_fs_fs__write_current(fs, 0, 1, 1, subpool));
+ }
+
+ uninitialize_fs_struct(fs);
+ svn_pool_destroy(subpool);
/* Now open the filesystem properly by calling the vtable method directly. */
return fs_open(fs, path, common_pool_lock, pool, common_pool);
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1629854&r1=1629853&r2=1629854&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Tue Oct 7 10:41:14 2014
@@ -86,7 +86,7 @@ are likely some errors because of that.
/* Declarations. */
static svn_error_t *
-get_youngest(svn_revnum_t *youngest_p, const char *fs_path, apr_pool_t *pool);
+get_youngest(svn_revnum_t *youngest_p, svn_fs_t *fs, apr_pool_t *pool);
/* Pathname helper functions */
@@ -213,7 +213,7 @@ with_some_lock_file(with_lock_baton_t *b
if (ffd->format >= SVN_FS_FS__MIN_PACKED_FORMAT)
err = svn_fs_fs__update_min_unpacked_rev(fs, pool);
if (!err)
- err = get_youngest(&ffd->youngest_rev_cache, fs->path, pool);
+ err = get_youngest(&ffd->youngest_rev_cache, fs, pool);
}
if (!err)
@@ -1131,7 +1131,7 @@ svn_fs_fs__open(svn_fs_t *fs, const char
/* Global configuration options. */
SVN_ERR(read_global_config(fs));
- return get_youngest(&(ffd->youngest_rev_cache), path, pool);
+ return get_youngest(&(ffd->youngest_rev_cache), fs, pool);
}
/* Wrapper around svn_io_file_create which ignores EEXIST. */
@@ -1310,17 +1310,11 @@ svn_fs_fs__upgrade(svn_fs_t *fs,
POOL. */
static svn_error_t *
get_youngest(svn_revnum_t *youngest_p,
- const char *fs_path,
+ svn_fs_t *fs,
apr_pool_t *pool)
{
- svn_stringbuf_t *buf;
- SVN_ERR(svn_fs_fs__read_content(&buf,
- svn_dirent_join(fs_path, PATH_CURRENT,
- pool),
- pool));
-
- *youngest_p = SVN_STR_TO_REV(buf->data);
-
+ apr_uint64_t dummy;
+ SVN_ERR(svn_fs_fs__read_current(youngest_p, &dummy, &dummy, fs, pool));
return SVN_NO_ERROR;
}
@@ -1332,7 +1326,7 @@ svn_fs_fs__youngest_rev(svn_revnum_t *yo
{
fs_fs_data_t *ffd = fs->fsap_data;
- SVN_ERR(get_youngest(youngest_p, fs->path, pool));
+ SVN_ERR(get_youngest(youngest_p, fs, pool));
ffd->youngest_rev_cache = *youngest_p;
return SVN_NO_ERROR;
@@ -1376,7 +1370,7 @@ svn_fs_fs__ensure_revision_exists(svn_re
if (rev <= ffd->youngest_rev_cache)
return SVN_NO_ERROR;
- SVN_ERR(get_youngest(&(ffd->youngest_rev_cache), fs->path, pool));
+ SVN_ERR(get_youngest(&(ffd->youngest_rev_cache), fs, pool));
/* Check again. */
if (rev <= ffd->youngest_rev_cache)
Re: svn commit: r1629854 - in
/subversion/trunk/subversion/libsvn_fs_fs: fs.c fs_fs.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Tue, Oct 07, 2014 at 15:02:41 +0200:
> On Tue, Oct 7, 2014 at 1:27 PM, Stefan Sperling <st...@elego.de> wrote:
> > On Tue, Oct 07, 2014 at 10:41:14AM -0000, stefan2@apache.org wrote:
> > > + /* Figure out the repo format and check that we can even handle it. */
> > > + SVN_ERR(svn_fs_fs__read_format_file(fs, subpool));
> > > +
> > > + /* Now, read 'current' and try to patch it if necessary. */
> > > + err = svn_fs_fs__youngest_rev(&youngest_rev, fs, subpool);
> > > + if (err)
> >
> > Can't we check for a specific error code here, and return the
> > error otherwise? This would make the intention of the error handling
> > code explicit and avoid masking of arbitrary error conditions.
> >
>
> If we wanted to enumerate all "unsurprising" error conditions,
> it might become quite a long list. After all, things are most
> likely broken when you run recover. To me, it seems best
> to try to get into a working state *despite* any previous errors.
> r1629879 tries to explain that.
I was also alarmed by the lack of specific error check, but my reasoning
for considering it okay was different: the worst-case if we unlink and
rewrite the 'current' file is that we do a bunch of work (namely,
figuring out youngest and highest-used-id by crawling the revs/ tree)
that we didn't actually have to do (e.g., because the error condition
was transient). So the failure mode is being inefficient^W
unnecessarily thorough, but not incorrectness.
There is a separate issue about malfunctions: when the API consumer
installs svn_error_raise_on_malfunction() as the malfunction handler,
any code that does
if (err)
may want to instead do
if (err && !SVN_ERROR_IN_CATEGORY(SVN_ERR_MALFUNC_CATEGORY_START))
so as to propagate the error immediately, without trying to handle it.
But this is a wider issue --- even the SVN_ERR() macro suffers from it.
Daniel
Re: svn commit: r1629854 - in /subversion/trunk/subversion/libsvn_fs_fs:
fs.c fs_fs.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Oct 7, 2014 at 1:27 PM, Stefan Sperling <st...@elego.de> wrote:
> On Tue, Oct 07, 2014 at 10:41:14AM -0000, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Tue Oct 7 10:41:14 2014
> > New Revision: 1629854
> >
> > URL: http://svn.apache.org/r1629854
> > Log:
> > In FSFS, always use the same function to read the 'current' file.
> >
> > Apart from the consistency aspect, this no longer lets atoi() mask
> > 'current' file corruptions. Recovery must be adopted to this.
>
> Hi Stefan,
>
> Two questions below:
>
> > --- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Tue Oct 7 10:41:14
> 2014
> > @@ -348,20 +349,47 @@ fs_open_for_recovery(svn_fs_t *fs,
> > apr_pool_t *pool,
> > apr_pool_t *common_pool)
> > {
> > + svn_error_t * err;
> > + svn_revnum_t youngest_rev;
> > + apr_pool_t * subpool = svn_pool_create(pool);
> > +
> > /* Recovery for FSFS is currently limited to recreating the 'current'
> > file from the latest revision. */
> >
> > /* The only thing we have to watch out for is that the 'current' file
> > - might not exist. So we'll try to create it here unconditionally,
> > - and just ignore any errors that might indicate that it's already
> > - present. (We'll need it to exist later anyway as a source for the
> > - new file's permissions). */
> > + might not exist or contain garbage. So we'll try to read it here
> > + and provide or replace the existing file if we couldn't read it.
> > + (We'll also need it to exist later anyway as a source for the new
> > + file's permissions). */
> >
> > - /* Use a partly-filled fs pointer first to create 'current'. This
> will fail
> > - if 'current' already exists, but we don't care about that. */
> > + /* Use a partly-filled fs pointer first to create 'current'. */
> > fs->path = apr_pstrdup(fs->pool, path);
> > - svn_error_clear(svn_io_file_create(svn_fs_fs__path_current(fs, pool),
> > - "0 1 1\n", pool));
> > +
> > + SVN_ERR(initialize_fs_struct(fs));
>
> The 'fs' struct is provided by the caller and is now initialised
> and uninitialised within this function. Can't this function
> use a local 'fs' variable? If not, why does it need to be uninitialised
> again? This is a bit confusing -- though perhaps it's an idiom used in
> the FS code that I'm not aware of?
>
The code would actually be nicer if it used a temporary FS.
After all, we are only trying to fix / prepare the on-disk data
before properly open the repo and trying to recover it.
However, svn_fs_new() is deprecated and there is no nice
alternative. We could use apr_pmemdup, write our own init
code or so but all these approaches are slightly fragile and
blur the lines between libsvn_fs and libsvn_fs_fs.
> > + /* Figure out the repo format and check that we can even handle it. */
> > + SVN_ERR(svn_fs_fs__read_format_file(fs, subpool));
> > +
> > + /* Now, read 'current' and try to patch it if necessary. */
> > + err = svn_fs_fs__youngest_rev(&youngest_rev, fs, subpool);
> > + if (err)
>
> Can't we check for a specific error code here, and return the
> error otherwise? This would make the intention of the error handling
> code explicit and avoid masking of arbitrary error conditions.
>
If we wanted to enumerate all "unsurprising" error conditions,
it might become quite a long list. After all, things are most
likely broken when you run recover. To me, it seems best
to try to get into a working state *despite* any previous errors.
r1629879 tries to explain that.
-- Stefan^2.
Re: svn commit: r1629854 - in
/subversion/trunk/subversion/libsvn_fs_fs: fs.c fs_fs.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 07, 2014 at 10:41:14AM -0000, stefan2@apache.org wrote:
> Author: stefan2
> Date: Tue Oct 7 10:41:14 2014
> New Revision: 1629854
>
> URL: http://svn.apache.org/r1629854
> Log:
> In FSFS, always use the same function to read the 'current' file.
>
> Apart from the consistency aspect, this no longer lets atoi() mask
> 'current' file corruptions. Recovery must be adopted to this.
Hi Stefan,
Two questions below:
> --- subversion/trunk/subversion/libsvn_fs_fs/fs.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs.c Tue Oct 7 10:41:14 2014
> @@ -348,20 +349,47 @@ fs_open_for_recovery(svn_fs_t *fs,
> apr_pool_t *pool,
> apr_pool_t *common_pool)
> {
> + svn_error_t * err;
> + svn_revnum_t youngest_rev;
> + apr_pool_t * subpool = svn_pool_create(pool);
> +
> /* Recovery for FSFS is currently limited to recreating the 'current'
> file from the latest revision. */
>
> /* The only thing we have to watch out for is that the 'current' file
> - might not exist. So we'll try to create it here unconditionally,
> - and just ignore any errors that might indicate that it's already
> - present. (We'll need it to exist later anyway as a source for the
> - new file's permissions). */
> + might not exist or contain garbage. So we'll try to read it here
> + and provide or replace the existing file if we couldn't read it.
> + (We'll also need it to exist later anyway as a source for the new
> + file's permissions). */
>
> - /* Use a partly-filled fs pointer first to create 'current'. This will fail
> - if 'current' already exists, but we don't care about that. */
> + /* Use a partly-filled fs pointer first to create 'current'. */
> fs->path = apr_pstrdup(fs->pool, path);
> - svn_error_clear(svn_io_file_create(svn_fs_fs__path_current(fs, pool),
> - "0 1 1\n", pool));
> +
> + SVN_ERR(initialize_fs_struct(fs));
The 'fs' struct is provided by the caller and is now initialised
and uninitialised within this function. Can't this function
use a local 'fs' variable? If not, why does it need to be uninitialised
again? This is a bit confusing -- though perhaps it's an idiom used in
the FS code that I'm not aware of?
> +
> + /* Figure out the repo format and check that we can even handle it. */
> + SVN_ERR(svn_fs_fs__read_format_file(fs, subpool));
> +
> + /* Now, read 'current' and try to patch it if necessary. */
> + err = svn_fs_fs__youngest_rev(&youngest_rev, fs, subpool);
> + if (err)
Can't we check for a specific error code here, and return the
error otherwise? This would make the intention of the error handling
code explicit and avoid masking of arbitrary error conditions.
> + {
> + const char *file_path;
> +
> + /* 'current' file is missing or contains garbage.
> + * Start with HEAD = 0 in that case. */
> + svn_error_clear(err);
> + file_path = svn_fs_fs__path_current(fs, subpool);
> +
> + /* Best effort to ensure the file exists and is valid.
> + * This may fail for r/o filesystems etc. */
> + SVN_ERR(svn_io_remove_file2(file_path, TRUE, subpool));
> + SVN_ERR(svn_io_file_create_empty(file_path, subpool));
> + SVN_ERR(svn_fs_fs__write_current(fs, 0, 1, 1, subpool));
> + }
> +
> + uninitialize_fs_struct(fs);
> + svn_pool_destroy(subpool);
>
> /* Now open the filesystem properly by calling the vtable method directly. */
> return fs_open(fs, path, common_pool_lock, pool, common_pool);
>