[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1146528 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Thu, 14 Jul 2011 18:54:59 +0300

I've looked again at the code. Right now it opens the db, with
svn_sqlite__mode_rwcreate, within an svn_atomic_init_once() block.

Adding the special case "For <this> caller use svn_sqlite__mode_readwrite"
means we can't use svn_atomic_init_once() any more. (since it's no
longer "once", maybe the first call failed due to being non-rwcreate)
And have to add some error handling logic around the open() call in that
function.

How about going for the less elegant but more readable solution,

[[[
Index: subversion/libsvn_fs_fs/rep-cache.c
===================================================================
--- subversion/libsvn_fs_fs/rep-cache.c (revision 1146757)
+++ subversion/libsvn_fs_fs/rep-cache.c (working copy)
@@ -43,8 +43,13 @@ REP_CACHE_DB_SQL_DECLARE_STATEMENTS(statements);
 
 
 /** Helper functions. **/
+static APR_INLINE const char *
+path_rep_cache_db(const char *fs_path,
+ apr_pool_t *result_pool)
+{
+ return svn_dirent_join(fs_path, REP_CACHE_DB_NAME, result_pool);
+}
 
-
 /* Check that REP refers to a revision that exists in FS. */
 static svn_error_t *
 rep_has_been_born(representation_t *rep,
@@ -91,7 +96,7 @@ open_rep_cache(void *baton,
 
   /* Open (or create) the sqlite database. It will be automatically
      closed when fs->pool is destoyed. */
- db_path = svn_dirent_join(fs->path, REP_CACHE_DB_NAME, pool);
+ db_path = path_rep_cache_db(fs->path, pool);
   SVN_ERR(svn_sqlite__open(&ffd->rep_cache_db, db_path,
                            svn_sqlite__mode_rwcreate, statements,
                            0, NULL,
@@ -182,6 +187,11 @@ svn_fs_fs__walk_rep_reference(svn_fs_t *fs,
 
       SVN_ERR(svn_sqlite__step(&have_row, stmt));
     }
+
+ if (iterations == 0)
+ SVN_ERR(svn_io_remove_file2(path_rep_cache_db(fs->path, iterpool),
+ TRUE /* ignore_enoent */,
+ iterpool));
 
   SVN_ERR(svn_sqlite__reset(stmt));
 
]]]

?

Julian Foad wrote on Thu, Jul 14, 2011 at 12:00:09 +0100:
> On Thu, 2011-07-14 at 04:10 +0300, Daniel Shahaf wrote:
> > Greg Stein wrote on Wed, Jul 13, 2011 at 20:54:56 -0400:
> > > On Wed, Jul 13, 2011 at 20:32, <danielsh_at_apache.org> wrote:
> > > > Author: danielsh
> > > > Date: Thu Jul 14 00:32:05 2011
> > > > New Revision: 1146528
> > > >
> > > > URL: http://svn.apache.org/viewvc?rev=1146528&view=rev
> > > > Log:
> > > > * subversion/libsvn_fs_fs/fs_fs.c
> > > > (write_config): Document that 'svnadmin verify' will access (and thus create)
> > > > rep-cache.db regardless of whether rep-sharing (of new reps) is enabled
> > > > in fsfs.conf.
> > >
> > > If rep-sharing is turned off, then it seems wrong to spontaneously
> > > create a .db that isn't begin used.
> >
> > It's harmless (it isn't being used).
>
> Harmless in functional terms only.
>
> > We could make the code pass readwrite (rather than rwcreate) for that
> > one caller, and then mask the error when the DB doesn't exist.
>
> +1.
>
> > (We do
> > want to verify the DB if it's exists but fsfs.conf says "Don't use it",
> > since fsfs.conf may get changed.)
>
> +1.
>
> > I'm not sure that non-creating an
> > unused db justifies this additional code.
>
> I'm with Greg - it would be good for the users.
>
> - Julian
>
>
Received on 2011-07-14 17:58:32 CEST

This is an archived mail posted to the Subversion Dev mailing list.