danderson@tigris.org writes:
> Followup to r15948. Fix stack smashing bugs and other smaller issues.
>
> Patch by: Stefan Küng <tortoisesvn@gmail.com>
> (tweaked by me)
> Suggested by: lundblad
> (proposed the solution to avoid circular dependancy)
Shouldn't this log message point out that this is the re-application
of r16010 and r16011, which were temporarily reverted in r16017?
I certainly got confused when I started reviewing and many the changes
looked eerily familiar... :-)
> --- trunk/subversion/libsvn_ra/ra_loader.c (original)
> +++ trunk/subversion/libsvn_ra/ra_loader.c Sat Sep 3 17:58:25 2005
> @@ -290,17 +306,18 @@
> {
> /* Deprecated function. Copy the contents of the svn_ra_callbacks_t
> to a new svn_ra_callbacks2_t and call svn_ra_open2(). */
> - svn_ra_callbacks2_t callbacks2;
> - 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;
> + svn_ra_callbacks2_t *callbacks2;
> + SVN_ERR (svn_ra_create_callbacks (&callbacks2, 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;
> return svn_ra_open2 (session_p, repos_URL,
> - &callbacks2, callback_baton,
> + callbacks2, callback_baton,
> config, pool);
> }
Aren't similar C structures guaranteed to be laid out exactly the same
from the beginning until their first point of difference? (I could
say that more formally, but you know what I mean.) What I'm getting
at is:
memcpy (callbacks2, callbacks, sizeof (*callbacks));
callbacks2->progress_func = NULL;
callbacks2->progress_baton = NULL;
See below...
> --- trunk/subversion/libsvn_ra/wrapper_template.h (original)
> +++ trunk/subversion/libsvn_ra/wrapper_template.h Sat Sep 3 17:58:25 2005
> @@ -45,20 +45,33 @@
> apr_pool_t *pool)
> {
> svn_ra_session_t *sess = apr_pcalloc (pool, sizeof (svn_ra_session_t));
> - svn_ra_callbacks2_t callbacks2;
> sess->vtable = &VTBL;
> sess->pool = pool;
> + /* Here, we should be calling svn_ra_create_callbacks to initialize
> + * the svn_ra_callbacks2_t structure. However, doing that
> + * introduces a circular dependancy between libsvn_ra and
> + * libsvn_ra_{local,dav,svn}, which include wrapper_template.h. In
> + * turn, circular dependancies break the build on win32 (and
> + * possibly other systems).
> + *
> + * In order to avoid this happening at all, the code of
> + * svn_ra_create_callbacks is duplicated here. This is evil, but
> + * the alternative (creating a new ra_util library) would be massive
> + * overkill for the time being. Just be sure to keep the following
> + * line and the code of svn_ra_create_callbacks in sync. */
> + svn_ra_callbacks2_t *callbacks2 = apr_pcalloc (pool,
> + sizeof (*callbacks2));
>
> - 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;
> + 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;
>
> - SVN_ERR (VTBL.open (sess, repos_URL, &callbacks2, callback_baton,
> + SVN_ERR (VTBL.open (sess, repos_URL, callbacks2, callback_baton,
> config, pool));
> *session_baton = sess;
> return SVN_NO_ERROR;
Similar comment applies here.
And the reason I bring it up is that it would reduce the amount of
duplicated code, not because I think it's a good way to do
compatibility structure copies in general.
> --- trunk/subversion/libsvn_ra_dav/session.c (original)
> +++ trunk/subversion/libsvn_ra_dav/session.c Sat Sep 3 17:58:25 2005
> @@ -561,14 +561,22 @@
> #endif /* if/else SVN_NEON_0_25 */
> }
>
> +typedef struct neonprogress_baton_t
> +{
> + svn_ra_progress_notify_func_t progress_func;
> + void *progress_baton;
> + apr_pool_t *pool;
> +} neonprogress_baton_t;
I had some comments in
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=105361
about this.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 20 03:17:01 2005