You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/03/28 19:47:04 UTC
RE: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
> -----Original Message-----
> From: philip@apache.org [mailto:philip@apache.org]
> Sent: vrijdag 28 maart 2014 18:51
> To: commits@subversion.apache.org
> Subject: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-
> loader.c
>
> Author: philip
> Date: Fri Mar 28 17:51:09 2014
> New Revision: 1582845
>
> URL: http://svn.apache.org/r1582845
> Log:
> Fix a potential error leak.
>
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_lock, svn_fs_unlock): Handle errors in a manner similar to
> the repos functions.
>
> Found by: julianfoad
>
> Modified:
> subversion/trunk/subversion/libsvn_fs/fs-loader.c
>
> Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs/fs-
> loader.c?rev=1582845&r1=1582844&r2=1582845&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs/fs-loader.c (original)
> +++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Fri Mar 28 17:51:09
> 2014
> @@ -1677,21 +1677,30 @@ svn_fs_lock(svn_lock_t **lock, svn_fs_t
> {
> apr_hash_t *targets = apr_hash_make(pool), *results;
> svn_fs_lock_target_t target;
> - svn_fs_lock_result_t *result;
> + svn_error_t *err;
>
> target.token = token;
> target.current_rev = current_rev;
> svn_hash_sets(targets, path, &target);
>
> - SVN_ERR(svn_fs_lock2(&results, fs, targets, comment, is_dav_comment,
> - expiration_date, steal_lock, pool, pool));
> + err = svn_fs_lock2(&results, fs, targets, comment, is_dav_comment,
> + expiration_date, steal_lock, pool, pool);
> +
> + if (apr_hash_count(results))
Is this function explicitly documented to always set the results value on all error paths?
I don't see that in the documentation for svn_fs_lock2?
And it is certainly not our standard pattern for public functions... By default the output arguments are undefined on error.
(It looks like all the implementations handle things this way...)
Bert
> + {
> + svn_fs_lock_result_t *result
> + = svn__apr_hash_index_val(apr_hash_first(pool, results));
>
> - SVN_ERR_ASSERT(apr_hash_count(results));
> - result = svn__apr_hash_index_val(apr_hash_first(pool, results));
> - if (result->lock)
> - *lock = result->lock;
> + if (result->lock)
> + *lock = result->lock;
> +
> + if (err && result->err)
> + svn_error_compose(err, result->err);
> + else if (!err)
> + err = result->err;
> + }
>
> - return result->err;
> + return err;
> }
>
> svn_error_t *
> @@ -1717,18 +1726,26 @@ svn_fs_unlock(svn_fs_t *fs, const char *
> svn_boolean_t break_lock, apr_pool_t *pool)
> {
> apr_hash_t *targets = apr_hash_make(pool), *results;
> - svn_fs_lock_result_t *result;
> + svn_error_t *err;
>
> if (!token)
> token = "";
> svn_hash_sets(targets, path, token);
>
> - SVN_ERR(svn_fs_unlock2(&results, fs, targets, break_lock, pool, pool));
> + err = svn_fs_unlock2(&results, fs, targets, break_lock, pool, pool);
> +
> + if (apr_hash_count(results))
> + {
> + svn_fs_lock_result_t *result
> + = svn__apr_hash_index_val(apr_hash_first(pool, results));
>
> - SVN_ERR_ASSERT(apr_hash_count(results));
> - result = svn__apr_hash_index_val(apr_hash_first(pool, results));
> + if (err && result->err)
> + svn_error_compose(err, result->err);
> + else if (!err)
> + err = result->err;
> + }
>
> - return result->err;
> + return err;
> }
>
> svn_error_t *
>
Re: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
> Perhaps it would be easier for all callers if the (inner) handler that
> allocates the hash table installs a cleanup handler on it that releases all
> the svn_error_t instances on pool cleanup of the release pool?
>
> Now all callers that have access to the results have to cover for the
> hopefully unlikely case where there are both global and per node errors,
> while such a cleanup handler would allow all of them to use the standard
> error pattern.
>
> (The code iterating the hash would also have to cover for errors further in
> the hash table, etc.)
I've been considering going back to a callback API, like the RA
function. I did start off with a callback API but as I don't want to
invoke the callback until the FS layer has released the repository
write-lock so the FS layer has to build up some sort of hash/array of
results. I decided to return the hash an let the caller decide how to
handle it, but we could iterate over that hash/array and invoke a
callback.
With a callback API the repos layer would intercept the FS callback and
use it to build a list of paths for the post- hook. Then the question
is can the repos layer invoke it's own callback before running the post-
hook or does it have to build up a hash/array and iterate after the
post- hook? Perhaps it does make sense to invoke the repos callback
before running the post- hook. The result of the post- hook is the
return value of the repos function.
A callback API would mean that the error handling is less prone to
leak. The FS layer would be responsible for clearing the per-path
errors after invoking the callback. The repos layer would simply pass
the error through to its own callback. The callers callback handler
would be able to inspect the error but would not clear it.
One question is what happens when the callback returns an error. I
suppose that stops further callbacks being invoked and eventually the
function returns the callback error back to the caller. Does it stop
the post- hook being invoked?
I've also been thinking about Julian's suggestion of some sort of hash
constructor to make locking/unlocking a single path easier. I think its
probably the wrong thing to do since the existing svn_fs_lock and
svn_fs_unlock are much better wrappers. I think the real questions are
whether svn_fs_lock should remain not deprecated and if so whether
svn_fs_lock2 should become svn_fs_lock_many.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
RE: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: vrijdag 28 maart 2014 20:57
> To: Bert Huijben
> Cc: dev@subversion.apache.org; commits@subversion.apache.org
> Subject: Re: svn commit: r1582845 -
> /subversion/trunk/subversion/libsvn_fs/fs-loader.c
>
> "Bert Huijben" <be...@qqmail.nl> writes:
>
> >> + expiration_date, steal_lock, pool, pool);
> >> +
> >> + if (apr_hash_count(results))
> >
> > Is this function explicitly documented to always set the results value
> > on all error paths?
> >
> > I don't see that in the documentation for svn_fs_lock2?
>
> It is now.
>
> > And it is certainly not our standard pattern for public
> > functions... By default the output arguments are undefined on error.
>
> It's necessary to allow the caller to handle all the errors.
Perhaps it would be easier for all callers if the (inner) handler that
allocates the hash table installs a cleanup handler on it that releases all
the svn_error_t instances on pool cleanup of the release pool?
Now all callers that have access to the results have to cover for the
hopefully unlikely case where there are both global and per node errors,
while such a cleanup handler would allow all of them to use the standard
error pattern.
(The code iterating the hash would also have to cover for errors further in
the hash table, etc.)
Bert
>
> --
> Philip Martin | Subversion Committer
> WANdisco // *Non-Stop Data*
RE: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: vrijdag 28 maart 2014 20:57
> To: Bert Huijben
> Cc: dev@subversion.apache.org; commits@subversion.apache.org
> Subject: Re: svn commit: r1582845 -
> /subversion/trunk/subversion/libsvn_fs/fs-loader.c
>
> "Bert Huijben" <be...@qqmail.nl> writes:
>
> >> + expiration_date, steal_lock, pool, pool);
> >> +
> >> + if (apr_hash_count(results))
> >
> > Is this function explicitly documented to always set the results value
> > on all error paths?
> >
> > I don't see that in the documentation for svn_fs_lock2?
>
> It is now.
>
> > And it is certainly not our standard pattern for public
> > functions... By default the output arguments are undefined on error.
>
> It's necessary to allow the caller to handle all the errors.
Perhaps it would be easier for all callers if the (inner) handler that
allocates the hash table installs a cleanup handler on it that releases all
the svn_error_t instances on pool cleanup of the release pool?
Now all callers that have access to the results have to cover for the
hopefully unlikely case where there are both global and per node errors,
while such a cleanup handler would allow all of them to use the standard
error pattern.
(The code iterating the hash would also have to cover for errors further in
the hash table, etc.)
Bert
>
> --
> Philip Martin | Subversion Committer
> WANdisco // *Non-Stop Data*
Re: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> + expiration_date, steal_lock, pool, pool);
>> +
>> + if (apr_hash_count(results))
>
> Is this function explicitly documented to always set the results value
> on all error paths?
>
> I don't see that in the documentation for svn_fs_lock2?
It is now.
> And it is certainly not our standard pattern for public
> functions... By default the output arguments are undefined on error.
It's necessary to allow the caller to handle all the errors.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1582845 - /subversion/trunk/subversion/libsvn_fs/fs-loader.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> + expiration_date, steal_lock, pool, pool);
>> +
>> + if (apr_hash_count(results))
>
> Is this function explicitly documented to always set the results value
> on all error paths?
>
> I don't see that in the documentation for svn_fs_lock2?
It is now.
> And it is certainly not our standard pattern for public
> functions... By default the output arguments are undefined on error.
It's necessary to allow the caller to handle all the errors.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*