You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2004/03/07 16:26:06 UTC
PATCH: error leaks
Are these fixes right, just clearing the errors, or should (some of) these errors be reported?
- Julian
Don't leak errors; handle or clear them.
* subversion/libsvn_fs/fs.c (svn_fs_create_berkeley, svn_fs_open_berkeley):
Clear any error that occurs within error-handling code.
* subversion/mod_dav_svn/repos.c (is_our_resource):
Clear any error that occurs, as there is no way to report it.
Index: subversion/libsvn_fs/fs.c
===================================================================
--- subversion/libsvn_fs/fs.c (revision 8913)
+++ subversion/libsvn_fs/fs.c (working copy)
@@ -582,7 +582,7 @@ svn_fs_create_berkeley (svn_fs_t *fs, co
return SVN_NO_ERROR;
error:
- (void) cleanup_fs (fs);
+ svn_error_clear (cleanup_fs (fs));
return svn_err;
}
@@ -654,7 +654,7 @@ svn_fs_open_berkeley (svn_fs_t *fs, cons
return SVN_NO_ERROR;
error:
- cleanup_fs (fs);
+ svn_error_clear (cleanup_fs (fs));
return svn_err;
}
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 8913)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -1363,23 +1363,23 @@ static int is_our_resource(const dav_res
if (res2->info->root.txn_name)
{
/* reopen the txn by name */
- (void) svn_fs_open_txn(&(res2->info->root.txn),
- res2->info->repos->fs,
- res2->info->root.txn_name,
- res2->info->repos->pool);
+ svn_error_clear (svn_fs_open_txn(&(res2->info->root.txn),
+ res2->info->repos->fs,
+ res2->info->root.txn_name,
+ res2->info->repos->pool));
/* regenerate the txn "root" object */
- (void) svn_fs_txn_root(&(res2->info->root.root),
- res2->info->root.txn,
- res2->info->repos->pool);
+ svn_error_clear (svn_fs_txn_root(&(res2->info->root.root),
+ res2->info->root.txn,
+ res2->info->repos->pool));
}
else if (res2->info->root.rev)
{
/* default: regenerate the revision "root" object */
- (void) svn_fs_revision_root(&(res2->info->root.root),
- res2->info->repos->fs,
- res2->info->root.rev,
- res2->info->repos->pool);
+ svn_error_clear (svn_fs_revision_root(&(res2->info->root.root),
+ res2->info->repos->fs,
+ res2->info->root.rev,
+ res2->info->repos->pool));
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: PATCH: error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>> Index: subversion/libsvn_fs/fs.c
>>===================================================================
>>--- subversion/libsvn_fs/fs.c (revision 8913)
>>+++ subversion/libsvn_fs/fs.c (working copy)
>>@@ -582,7 +582,7 @@ svn_fs_create_berkeley (svn_fs_t *fs, co
>> return SVN_NO_ERROR;
>> error:
>>- (void) cleanup_fs (fs);
>>+ svn_error_clear (cleanup_fs (fs));
>
> Your change is OK, but there is an SVN_ERR in the function that could
> bypass the error: label. I really think Subversion should avoid this
> particular goto construct as it's too easy to introduce an SVN_ERR,
> but that doesn't mean you should not apply your patch.
Yes - yuck.
That's a separate issue, so committed this patch as r8923. Thanks for the review.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: PATCH: error leaks
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@btopenworld.com> writes:
> Index: subversion/libsvn_fs/fs.c
> ===================================================================
> --- subversion/libsvn_fs/fs.c (revision 8913)
> +++ subversion/libsvn_fs/fs.c (working copy)
> @@ -582,7 +582,7 @@ svn_fs_create_berkeley (svn_fs_t *fs, co
> return SVN_NO_ERROR;
> error:
> - (void) cleanup_fs (fs);
> + svn_error_clear (cleanup_fs (fs));
Your change is OK, but there is an SVN_ERR in the function that could
bypass the error: label. I really think Subversion should avoid this
particular goto construct as it's too easy to introduce an SVN_ERR,
but that doesn't mean you should not apply your patch.
> return svn_err;
> }
> @@ -654,7 +654,7 @@ svn_fs_open_berkeley (svn_fs_t *fs, cons
> return SVN_NO_ERROR;
> error:
> - cleanup_fs (fs);
> + svn_error_clear (cleanup_fs (fs));
ditto
> return svn_err;
> }
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org