You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/11/16 18:25:39 UTC
Re: svn commit: r1202783 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
On Wednesday, November 16, 2011 5:06 PM, philip@apache.org wrote:
> Author: philip
> Date: Wed Nov 16 17:06:50 2011
> New Revision: 1202783
>
> URL: http://svn.apache.org/viewvc?rev=1202783&view=rev
> Log:
> * subversion/libsvn_fs_fs/fs_fs.c
> (rep_write_contents_close): Log root cause, rather than a generic error,
> when failing to open rep-cache.db.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>
> 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=1202783&r1=1202782&r2=1202783&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Nov 16 17:06:50 2011
> @@ -5600,7 +5600,7 @@ rep_write_contents_close(void *baton)
> /* Something's wrong with the rep-sharing index. We can continue
> without rep-sharing, but warn.
> */
> - (b->fs->warning)(b->fs->warning_baton, err);
> + (b->fs->warning)(b->fs->warning_baton, svn_error_root_cause(err));
Disagree. This just loses information. The implementation of
svn_fs_warning_func_t in mod_dav_svn should call
svn_error_root_cause() if it so chooses, but let's not prevent the
chain from callers who care about it.
Accordingly, -0 to backport.
> svn_error_clear(err);
> old_rep = NULL;
> }
>
>
>
Re: svn commit: r1202783 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Thursday, November 17, 2011 11:05 AM, "Philip Martin" <ph...@wandisco.com> wrote:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > I suggested on IRC to make the code record the complete error chain.
>
> Would we do that in the logging handlers in mod_dav_svn and svnserve,
> and thus do it for every error that is logged? Or would we get the
> calling code in libsvn_fs_fs to invoke the logging handler multiple
> times?
I had in mind the former. I don't see why we would ever want to invoke
fs->warning_func() multiple times; it seems sufficient for me to invoke
it once, with the complete error chain, and let the callback
implementation handle the chain, or parts thereof, as it sees fit.
>
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com
>
Re: svn commit: r1202783 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> I suggested on IRC to make the code record the complete error chain.
Would we do that in the logging handlers in mod_dav_svn and svnserve,
and thus do it for every error that is logged? Or would we get the
calling code in libsvn_fs_fs to invoke the logging handler multiple
times?
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1202783 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Wed, Nov 16, 2011 at 18:56:56 +0000:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> >> fs_fs knows that in this case the root cause is important and that the
> >> error chain is mostly useless. mod_dav_svn, and svnserve, cannot know
> >> that so would have to print the whole chain in every case.
> >
> > "In this case"? How do you know what the case is? It might be a disk
> > problem, not just the specific error that triggered your fix.
>
> Getting the rep from the rep-cache database failed. The root error is
> the one that matters, whether it is "permission denied" or "wrong sqlite
> version" or "i/o error". Recording "failed to open database" is not
> useful.
>
On the other hand, recording "Permission denied" without also recording
that it's related to the rep-cache DB, or to a particular rev file, is
also an instance of removing relevant information.
I suggested on IRC to make the code record the complete error chain.
> > Should we add some SQLite error code to the if() condition whose else()
> > branch you patched? For example, "SQLite db is corrupted" would appear
> > to be serious enough (if it's not already caught by the
> > SVN_ERR_FS_CORRUPT check).
>
> That branch doesn't clear the error, I assume it gets handled elsewhere.
Pretty much nothing catches SVN_ERR_FS_CORRUPT and assertion errors, so
I expect errors caught in the if() branch propagate all the way to
mod_dav_svn, if not to mod_dav itself.
Re: svn commit: r1202783 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
>> fs_fs knows that in this case the root cause is important and that the
>> error chain is mostly useless. mod_dav_svn, and svnserve, cannot know
>> that so would have to print the whole chain in every case.
>
> "In this case"? How do you know what the case is? It might be a disk
> problem, not just the specific error that triggered your fix.
Getting the rep from the rep-cache database failed. The root error is
the one that matters, whether it is "permission denied" or "wrong sqlite
version" or "i/o error". Recording "failed to open database" is not
useful.
> Should we add some SQLite error code to the if() condition whose else()
> branch you patched? For example, "SQLite db is corrupted" would appear
> to be serious enough (if it's not already caught by the
> SVN_ERR_FS_CORRUPT check).
That branch doesn't clear the error, I assume it gets handled elsewhere.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1202783 -
/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Wed, Nov 16, 2011 at 17:31:12 +0000:
> "Daniel Shahaf" <d....@daniel.shahaf.name> writes:
>
> > On Wednesday, November 16, 2011 5:06 PM, philip@apache.org wrote:
> >> Author: philip
> >> Date: Wed Nov 16 17:06:50 2011
> >> New Revision: 1202783
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1202783&view=rev
> >> Log:
> >> * subversion/libsvn_fs_fs/fs_fs.c
> >> (rep_write_contents_close): Log root cause, rather than a generic error,
> >> when failing to open rep-cache.db.
> >>
> >> Modified:
> >> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >>
> >> 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=1202783&r1=1202782&r2=1202783&view=diff
> >> ==============================================================================
> >> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Nov 16 17:06:50 2011
> >> @@ -5600,7 +5600,7 @@ rep_write_contents_close(void *baton)
> >> /* Something's wrong with the rep-sharing index. We can continue
> >> without rep-sharing, but warn.
> >> */
> >> - (b->fs->warning)(b->fs->warning_baton, err);
> >> + (b->fs->warning)(b->fs->warning_baton, svn_error_root_cause(err));
> >
> > Disagree. This just loses information. The implementation of
> > svn_fs_warning_func_t in mod_dav_svn should call
> > svn_error_root_cause() if it so chooses, but let's not prevent the
> > chain from callers who care about it.
> >
> > Accordingly, -0 to backport.
>
> fs_fs knows that in this case the root cause is important and that the
> error chain is mostly useless. mod_dav_svn, and svnserve, cannot know
> that so would have to print the whole chain in every case.
"In this case"? How do you know what the case is? It might be a disk
problem, not just the specific error that triggered your fix.
Should we add some SQLite error code to the if() condition whose else()
branch you patched? For example, "SQLite db is corrupted" would appear
to be serious enough (if it's not already caught by the
SVN_ERR_FS_CORRUPT check).
Re: svn commit: r1202783 - /subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
Posted by Philip Martin <ph...@wandisco.com>.
"Daniel Shahaf" <d....@daniel.shahaf.name> writes:
> On Wednesday, November 16, 2011 5:06 PM, philip@apache.org wrote:
>> Author: philip
>> Date: Wed Nov 16 17:06:50 2011
>> New Revision: 1202783
>>
>> URL: http://svn.apache.org/viewvc?rev=1202783&view=rev
>> Log:
>> * subversion/libsvn_fs_fs/fs_fs.c
>> (rep_write_contents_close): Log root cause, rather than a generic error,
>> when failing to open rep-cache.db.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>>
>> 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=1202783&r1=1202782&r2=1202783&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Nov 16 17:06:50 2011
>> @@ -5600,7 +5600,7 @@ rep_write_contents_close(void *baton)
>> /* Something's wrong with the rep-sharing index. We can continue
>> without rep-sharing, but warn.
>> */
>> - (b->fs->warning)(b->fs->warning_baton, err);
>> + (b->fs->warning)(b->fs->warning_baton, svn_error_root_cause(err));
>
> Disagree. This just loses information. The implementation of
> svn_fs_warning_func_t in mod_dav_svn should call
> svn_error_root_cause() if it so chooses, but let's not prevent the
> chain from callers who care about it.
>
> Accordingly, -0 to backport.
fs_fs knows that in this case the root cause is important and that the
error chain is mostly useless. mod_dav_svn, and svnserve, cannot know
that so would have to print the whole chain in every case.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com