You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@xbc.nu> on 2005/08/31 01:55:12 UTC
Re: svn commit: r16010 - in trunk/subversion: libsvn_ra libsvn_ra_dav
danderson@tigris.org wrote:
>Author: danderson
>Date: Tue Aug 30 18:58:16 2005
>New Revision: 16010
>
>Modified:
> trunk/subversion/include/svn_ra.h
> trunk/subversion/libsvn_ra/ra_loader.c
> trunk/subversion/libsvn_ra/wrapper_template.h
> trunk/subversion/libsvn_ra_dav/session.c
>
>Log:
>Followup to r15948. Fix stack smashing bugs and other smaller issues.
>
>Patch by: Stefan Küng <to...@gmail.com>
>Review by: danderson
>(Patch also tweaked by me)
>
>* 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): Use the new svn_ra_create_callbacks() function.
>
>* subversion/libsvn_ra_dav/session.c
> (neon_progress_baton_t): New structure to pass to the neon callback.
> (ra_dav_neonprogress): Pass the new apr_pool_t param to the progress
> callback.
> (svn_ra_dav__open): Use the new neon_progress_baton_t struct to pass
> to the neon callback function.
>
>
>Modified: trunk/subversion/include/svn_ra.h
>Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/include/svn_ra.h?rev=16010&p1=trunk/subversion/include/svn_ra.h&p2=trunk/subversion/include/svn_ra.h&r1=16009&r2=16010
>==============================================================================
>--- trunk/subversion/include/svn_ra.h (original)
>+++ trunk/subversion/include/svn_ra.h Tue Aug 30 18:58:16 2005
>@@ -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.
>
s/In order to ease future compatibility, c/C/
>+/** 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.
>
>
s/In order to easy future compatibility, c/C/
And notice the typo (easy -> ease)
>http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_dav/session.c?rev=16010&p1=trunk/subversion/libsvn_ra_dav/session.c&p2=trunk/subversion/libsvn_ra_dav/session.c&r1=16009&r2=16010
>==============================================================================
>--- trunk/subversion/libsvn_ra_dav/session.c (original)
>+++ trunk/subversion/libsvn_ra_dav/session.c Tue Aug 30 18:58:16 2005
>@@ -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;
>+ apr_pool_t *pool;
>+} neonprogress_baton_t;
>
>
In svn_ra_callbacks2_t and svn_client_context_t, progress_func is before
progress_baton. Minor nit here, but consistency is nice.
> static void
>-ra_dav_neonprogress(void * baton, off_t progress, off_t total)
>+ra_dav_neonprogress(void *baton, off_t progress, off_t total)
> {
>- /* Important: don't change the svn_ra_callbacks2_t struct here! */
>- const svn_ra_callbacks2_t * callbacks = (svn_ra_callbacks2_t *)baton;
>- if (callbacks->progress_func)
>+ const neonprogress_baton_t *neonprogress_baton =
>+ (neonprogress_baton_t *)baton;
>
>
No-op cast from void*
>+ if (neonprogress_baton->progress_func)
> {
>- callbacks->progress_func(progress, total, callbacks->progress_baton);
>+ neonprogress_baton->progress_func(progress, total,
>+ neonprogress_baton->progress_baton,
>+ neonprogress_baton->pool);
> }
> }
>
>@@ -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));
>
>
Don't don't don't don't use sizeof(type) for allocation. Use
sizeof(variable) instead, to avoid grief if the variable's type changes
but you forget to change the sizeof, too. Like this:
neonprogress_baton_t *neonprogress_baton =
apr_pcalloc(pool, sizeof(*neonprogress_baton));
>
> /* Sanity check the URI */
> if (ne_uri_parse(repos_URL, &uri)
>@@ -802,13 +813,11 @@
> ne_ssl_trust_default_ca(sess2);
> }
> }
>-
>- /* Set the neon callback to make it call the svn_progress_notify_func_t
>- * Note that ne_set_progress() takes a non-const baton as the third param.
>- * Since we don't change the callback struct but only use the non-const
>- * notification callback items of that struct, it's safe to cast */
>- ne_set_progress(sess, ra_dav_neonprogress, (void*)callbacks);
>- ne_set_progress(sess2, ra_dav_neonprogress, (void*)callbacks);
>+ neonprogress_baton->pool = pool;
>+ neonprogress_baton->progress_baton = callbacks->progress_baton;
>+ neonprogress_baton->progress_func = callbacks->progress_func;
>+ ne_set_progress(sess, ra_dav_neonprogress, (void*)neonprogress_baton);
>+ ne_set_progress(sess2, ra_dav_neonprogress, (void*)neonprogress_baton);
> session->priv = ras;
>
>
No-op casts to void*. Get rid of them. They were necessary before
because they also cast away the const on the callbacks baton, but are no
longer needed now.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16010 - in trunk/subversion: libsvn_ra libsvn_ra_dav
Posted by David Anderson <da...@calixo.net>.
Branko Čibej wrote:
> s/In order to ease future compatibility, c/C/
For these two, I personally agree, but see the discussion in the other
thread about this.
> No-op cast from void*
> Don't don't don't don't use sizeof(type) for allocation.
> No-op casts to void*.
Fixed. Once a decision on the doc is made, I'll commit this.
- Dave.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org