Bert Huijben wrote on Tue, Aug 03, 2010 at 17:43:38 +0200:
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:d...@daniel.shahaf.name]
> > Sent: dinsdag 3 augustus 2010 17:20
> > To: dev@subversion.apache.org
> > Subject: Re: opening fsfs rev files rw
> >
> > Daniel Shahaf wrote on Tue, Aug 03, 2010 at 11:58:32 +0300:
> > > There was yesterday on IRC [1] some discussion around whether revprop
> > > editing can corrupt rev files.
> > >
> > > [[[
> > > 0:% rm -rf r
> > > 0:% ./subversion/svnadmin/svnadmin create r
> > > 0:% ls -l r/db/revs/0/0
> > > -rw-r--r-- 1 daniel daniel 115 2010-08-03 11:56 r/db/revs/0/0
> > > ]]]
> > >
> > > Shouldn't we create rev files with the write bit disabled? (i.e.,
> > 0222 umask)
> > >
> >
> > r981921 + nominated for backport.
>
> I'm not sure whether this really helps. What I suggested on IRC, was that we
> shouldn't open the files for read-write, but only for reading (where I just
> tried to get confirmation that we did)
>
To answer your question, open_pack_or_rev_file() does pass APR_READ.
(I grepped for 'open\i*(', where \i is [a-z0-9_].)
But yesterday while debugging this (via a couple of 'strace | grep' and
'chmod; svn pe --revprop' invocations), I noticed that we weren't marking
rev files rw on disk. Since they are immutable, it seemed worthwhile
(a small improvement) to mark them ro.
> This patch just makes us a tiny bit slower and makes it a bit harder for
> third party tools to corrupt the repository.
>
Slower how?
>
> One other thing: How does 'svnadmin pack' handle this?
>
> If it can't handle this we might have to delay this to the first format
> bump.
>
Pack and manifest files are made read-only too (explicitly in the code;
their permissions are not inherited from the permissions of rev files).
Old rev shards are removed when packed. Does that answer your question?
> Bert
>
