Return-Path: X-Original-To: apmail-subversion-dev-archive@minotaur.apache.org Delivered-To: apmail-subversion-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 4C8E017AEB for ; Tue, 7 Oct 2014 13:03:08 +0000 (UTC) Received: (qmail 92877 invoked by uid 500); 7 Oct 2014 13:03:08 -0000 Delivered-To: apmail-subversion-dev-archive@subversion.apache.org Received: (qmail 92821 invoked by uid 500); 7 Oct 2014 13:03:08 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 92811 invoked by uid 99); 7 Oct 2014 13:03:07 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 07 Oct 2014 13:03:07 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of stefan.fuhrmann@wandisco.com designates 209.85.213.181 as permitted sender) Received: from [209.85.213.181] (HELO mail-ig0-f181.google.com) (209.85.213.181) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 07 Oct 2014 13:03:03 +0000 Received: by mail-ig0-f181.google.com with SMTP id r10so4606522igi.14 for ; Tue, 07 Oct 2014 06:02:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wandisco.com; s=gapps; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=3p4lTsqaYo17DeP4rIABvB7ZS7Z00fBckBrQfE1s3oY=; b=Riqwmk/jAnjl5lvQ20EH9/t64tMqri2oPhVstysGoG34SDrp1+o0IjWp2w0IwNPBF9 L7Z9HYnWceNRKQWXeDi4TzWL9Nd+FCalFQaPwByc5T9cABzM7tD00fyHc2Z4xDbMo+vA 29yH4iD2dDyb1mUQTwgGQdBUddRE31iUELJTE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:content-type; bh=3p4lTsqaYo17DeP4rIABvB7ZS7Z00fBckBrQfE1s3oY=; b=IlRzbwXSxc0CkHugpp1O6GkWel+3LwwzxazLnfpWhUVQwCiuuP3zj4Z92uP/kkWkA9 Hoi9pNqJ9PUSz3caXpxa4eNGaQNJIW8k58xk2qWtLxT98iSOLftAGnIX7832vSyO3OSh Ic8PiDNJ9iKKW47w4OdGfrlza6EYixzLeQq7aqA6F2RLPilboPBYpzHklozIoEbHvo9P 51PU0tW15jsQ9TnzbI5mAz9ufkUIMKobMNFEVcRGawlVW1Jr0K8U/TnAUQrJyxr1ssX5 6vZGlfD+QYbjMCAcYuR7ycPemTA0jALjS0cRDLx9Aet7K+yJJoM8RiVSeLJzOZgRW8Zp fRaQ== X-Gm-Message-State: ALoCoQm8mnZewY7ALXqi89eTSsLbHTxC2ZmhFDB9EkPQ1HL6hGa9riBPFKfEfToR8qj04tS5NTGv MIME-Version: 1.0 X-Received: by 10.50.73.200 with SMTP id n8mr32091524igv.16.1412686961235; Tue, 07 Oct 2014 06:02:41 -0700 (PDT) Received: by 10.50.214.101 with HTTP; Tue, 7 Oct 2014 06:02:41 -0700 (PDT) In-Reply-To: <20141007112748.GC17513@ted.stsp.name> References: <20141007104114.C0A3D2388993@eris.apache.org> <20141007112748.GC17513@ted.stsp.name> Date: Tue, 7 Oct 2014 15:02:41 +0200 Message-ID: Subject: Re: svn commit: r1629854 - in /subversion/trunk/subversion/libsvn_fs_fs: fs.c fs_fs.c From: Stefan Fuhrmann To: Subversion Development Content-Type: multipart/alternative; boundary=089e013a22708134160504d4d079 X-Virus-Checked: Checked by ClamAV on apache.org --089e013a22708134160504d4d079 Content-Type: text/plain; charset=UTF-8 On Tue, Oct 7, 2014 at 1:27 PM, Stefan Sperling 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. --089e013a22708134160504d4d079 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On T= ue, Oct 7, 2014 at 1:27 PM, Stefan Sperling <stsp@elego.de> wr= ote:
On Tue, Oct 07, 2= 014 at 10:41:14AM -0000, stefan2@apac= he.org wrote:
> Author: stefan2
> Date: Tue Oct=C2=A0 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' fi= le.
>
> Apart from the consistency aspect, this no longer lets atoi() mask
> 'current' file corruptions.=C2=A0 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=C2=A0 7 10:4= 1:14 2014
> @@ -348,20 +349,47 @@ fs_open_for_recovery(svn_fs_t *fs,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0apr_pool_t *pool,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0apr_pool_t *common_pool)
>=C2=A0 {
> +=C2=A0 svn_error_t * err;
> +=C2=A0 svn_revnum_t youngest_rev;
> +=C2=A0 apr_pool_t * subpool =3D svn_pool_create(pool);
> +
>=C2=A0 =C2=A0 /* Recovery for FSFS is currently limited to recreating t= he 'current'
>=C2=A0 =C2=A0 =C2=A0 =C2=A0file from the latest revision. */
>
>=C2=A0 =C2=A0 /* The only thing we have to watch out for is that the &#= 39;current' file
> -=C2=A0 =C2=A0 =C2=A0might not exist.=C2=A0 So we'll try to create= it here unconditionally,
> -=C2=A0 =C2=A0 =C2=A0and just ignore any errors that might indicate th= at it's already
> -=C2=A0 =C2=A0 =C2=A0present. (We'll need it to exist later anyway= as a source for the
> -=C2=A0 =C2=A0 =C2=A0new file's permissions). */
> +=C2=A0 =C2=A0 =C2=A0might not exist or contain garbage.=C2=A0 So we&#= 39;ll try to read it here
> +=C2=A0 =C2=A0 =C2=A0and provide or replace the existing file if we co= uldn't read it.
> +=C2=A0 =C2=A0 =C2=A0(We'll also need it to exist later anyway as = a source for the new
> +=C2=A0 =C2=A0 =C2=A0file's permissions). */
>
> -=C2=A0 /* Use a partly-filled fs pointer first to create 'current= '.=C2=A0 This will fail
> -=C2=A0 =C2=A0 =C2=A0if 'current' already exists, but we don&#= 39;t care about that. */
> +=C2=A0 /* Use a partly-filled fs pointer first to create 'current= '. */
>=C2=A0 =C2=A0 fs->path =3D apr_pstrdup(fs->pool, path);
> -=C2=A0 svn_error_clear(svn_io_file_create(svn_fs_fs__path_current(fs,= pool),
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"0 1 1\= n", pool));
> +
> +=C2=A0 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 uninitial= ised
again? This is a bit confusing -- though perhaps it's an idiom used in<= br> the FS code that I'm not aware of?

= The code would actually be nicer if it used a temporary FS.
After all, w= e 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 li= bsvn_fs_fs.
=C2=A0
> +=C2=A0 /* Figure out the repo format and check that we c= an even handle it. */
> +=C2=A0 SVN_ERR(svn_fs_fs__read_format_file(fs, subpool));
> +
> +=C2=A0 /* Now, read 'current' and try to patch it if necessar= y. */
> +=C2=A0 err =3D svn_fs_fs__youngest_rev(&youngest_rev, fs, subpool= );
> +=C2=A0 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&qu= ot; error conditions,
it might become quite a long list. After all, thin= gs are most
likely broken when you run recover. To me, it see= ms best
to try to get into a working state *despite* any previous errors= .
r1629879 tries to explain that.
=
-- Stefan^2.
--089e013a22708134160504d4d079--