I've actually been working on these fixups in my own wc. Here is my
review nonetheless. No need to resubmit the fix patch.
Stefan Küng wrote:
> [[[
> Followup to r15948. Fix stack smashing bugs and other smaller issues.
>
> Patch by: Stefan Küng <tortoisesvn@gmail.com> with input from brane and
> danderson.
The contribution tracking guidelines say to put extra details on a
line below the actual parseable fields of the log. Also, it is
actually lundblad who suggested the fix you're implementing:
Patch by: Stefan Küng <tortoisesvn@gmail.com>
(tweaked by me)
Suggested by: lundblad
(proposed the solution to avoid circular dependancy)
I'm not sure brane and I should be included in this list, as our input
has really been reviewing of your previous changeset so far. Opinions
by other commiters on this is welcome, but to me it sounds like a very
http://aquamarine.bikeshed.com/ .
> * subversion/include/svn_ra.h
> (svn_ra_progress_notify_func_t): Add an apr_pool_t to the callback
> prototype.
> (svn_ra_create_callbacks): New function to create and initialize the
> svn_ra_callbacks2_t object.
> (svn_ra_callbacks2_t): Add comment about svn_ra_create_callbacks().
> (svn_ra_callbacks_t): Change comment.
>
> * subversion/libsvn_ra/ra_loader.c
> (svn_ra_create_callbacks): New function to create and initialize
> the svn_ra_callbacks2_t object.
> (svn_ra_open): Use the new svn_ra_create_callbacks().
>
> * subversion/libsvn_ra/wrapper_template.h
> (compat_open): Allocate memory for the svn_ra_callbacks2_t object
> from pool.
A short reason here would be nice: "Allocate the svn_ra_callbacks2_t
directly, to avoid introducing a circular dependancy."
> Index: subversion/libsvn_ra/wrapper_template.h
> ===================================================================
> --- subversion/libsvn_ra/wrapper_template.h (Revision 16034)
> +++ subversion/libsvn_ra/wrapper_template.h (Arbeitskopie)
> @@ -45,20 +45,22 @@
> apr_pool_t *pool)
> {
> svn_ra_session_t *sess = apr_pcalloc (pool, sizeof
(svn_ra_session_t));
> - svn_ra_callbacks2_t callbacks2;
> + svn_ra_callbacks2_t *callbacks2 = NULL;
Why set it to NULL? You could allocate at this point directly.
> sess->vtable = &VTBL;
> sess->pool = pool;
>
> - callbacks2.open_tmp_file = callbacks->open_tmp_file;
> - callbacks2.auth_baton = callbacks->auth_baton;
> - callbacks2.get_wc_prop = callbacks->get_wc_prop;
> - callbacks2.set_wc_prop = callbacks->set_wc_prop;
> - callbacks2.push_wc_prop = callbacks->push_wc_prop;
> - callbacks2.invalidate_wc_props = callbacks->invalidate_wc_props;
> - callbacks2.progress_func = NULL;
> - callbacks2.progress_baton = NULL;
> + callbacks = apr_pcalloc (pool, sizeof (svn_ra_callbacks2_t));
Oops, that should be callbacks2, not callbacks! Also, Branko suggested
allocating with sizeof(*callbacks2), to avoid problems down the road
when changing the variable type.
> Index: subversion/libsvn_ra/ra_loader.c
> ===================================================================
> --- subversion/libsvn_ra/ra_loader.c (Revision 16034)
> +++ subversion/libsvn_ra/ra_loader.c (Arbeitskopie)
> @@ -230,6 +230,21 @@
> return SVN_NO_ERROR;
> }
>
> +svn_error_t *
> +svn_ra_create_callbacks (svn_ra_callbacks2_t **callbacks,
> + apr_pool_t *pool)
> +{
> + /* Note: if you change this function, you also must change the
> + allocation of the svn_ra_callbacks2_t objects in
> + libsvn_ra/wrapper_template:compat_open()
> +
> + The reason that those functions don't use svn_ra_create_callbacks
> + is to avoid circular dependencies with include files.
> + */
Two extremely minor nits: "must also change" flows better than "also
must change", and "object" instead of "objects" -- only one structure
is allocated. Also, the second paragraph could be more precise about
which include files, which libraries etc. play a part in the problem
being solved.
Furthermore, a similar comment should be placed in wrapper_template.h,
so that someone modifying compat_open() doesn't accidentally miss the
issue completely.
> + *callbacks = apr_pcalloc (pool, sizeof (svn_ra_callbacks2_t));
Same comment as above about the sizeof() argument.
> Index: subversion/include/svn_ra.h
> ===================================================================
> --- subversion/include/svn_ra.h (Revision 16034)
> +++ subversion/include/svn_ra.h (Arbeitskopie)
> @@ -180,7 +180,8 @@
> */
> typedef void (*svn_ra_progress_notify_func_t) (apr_off_t progress,
> apr_off_t total,
> - void * baton);
> + void *baton,
> + apr_pool_t *pool);
>
>
> /**
> @@ -322,6 +323,10 @@
> * Each routine takes a @a callback_baton originally provided with the
> * vtable.
> *
> + * In order to ease future compatibility, clients must use
> + * svn_ra_create_callbacks() to allocate and initialize this structure
> + * instead of doing so themselves.
> + *
No need to explain the implementation reasons in the public API, as
discussed in previous commit reviews. "Clients must use
svn_ra_create_callbacks() to allocate and initialize this structure."
is enough for the public API documentation.
> @@ -439,6 +417,19 @@
> svn_error_t *
> svn_ra_initialize (apr_pool_t *pool);
>
> +/** Initialize a callback structure.
> +* Set @a *callbacks to a ra callbacks object, allocated in @a pool.
> +*
> +* In order to easy future compatibility, clients must use this
> +* function to allocate and initialize the @c svn_ra_callbacks2_t
> +* structure rather than doing so themselves.
Same comment as above, this sentence can be reduced to "Clients must
use this function to allocate and initialize @c svn_ra_callbacks2_t
structures."
> Index: subversion/libsvn_ra_dav/session.c
> ===================================================================
> --- subversion/libsvn_ra_dav/session.c (Revision 16034)
> +++ subversion/libsvn_ra_dav/session.c (Arbeitskopie)
> @@ -561,14 +561,23 @@
> #endif /* if/else SVN_NEON_0_25 */
> }
>
> +typedef struct neonprogress_baton_t
> +{
> + void *progress_baton;
> + svn_ra_progress_notify_func_t progress_func;
Another of Branko's minor nits here -- these two fields should be
switched, for consistency with the rest of the API.
> + const neonprogress_baton_t *neonprogress_baton =
> + (neonprogress_baton_t *)baton;
No-op cast from void*, not needed.
> @@ -593,6 +602,8 @@
> svn_boolean_t compression;
> svn_config_t *cfg;
> const char *server_group;
> + neonprogress_baton_t *neonprogress_baton =
> + apr_pcalloc(pool, sizeof(neonprogress_baton_t));
Use the variable name, not the structure name, in the allocation
sizeof().
> + ne_set_progress(sess, ra_dav_neonprogress, (void*)neonprogress_baton);
> + ne_set_progress(sess2, ra_dav_neonprogress,
(void*)neonprogress_baton);
No-op casts to void*, remove.
Mostly the same minor nits that Branko had already found in previous
commits. Like I said, I had already corrected this in my working
copy. The code compiles without error, and my test program (svnput)
no longer segfaults using the legacy API, so all looks well (sorry,
this last sentence is mostly for me - making sure everything is okay
before I commit :-) ).
Hoping that it'll be longer lived than last time round, I've commited
the fix as r16037. I'll give you a review on the other patch in a mail
I've just started. Thanks for the contribution!
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Sep 4 01:14:48 2005