You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2012/10/05 04:03:41 UTC
Re: svn commit: r1394346 - /subversion/trunk/subversion/libsvn_ra_svn/client.c
On 10/04/2012 09:55 PM, stefan2@apache.org wrote:
> Author: stefan2
> Date: Fri Oct 5 01:54:59 2012
> New Revision: 1394346
>
> URL: http://svn.apache.org/viewvc?rev=1394346&view=rev
> Log:
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_commit): initialize *all* members of the ccb struct
>
> Modified:
> subversion/trunk/subversion/libsvn_ra_svn/client.c
>
> Modified: subversion/trunk/subversion/libsvn_ra_svn/client.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/client.c?rev=1394346&r1=1394345&r2=1394346&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_svn/client.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_svn/client.c Fri Oct 5 01:54:59 2012
> @@ -1044,6 +1044,7 @@ static svn_error_t *ra_svn_commit(svn_ra
> ccb = apr_palloc(pool, sizeof(*ccb));
> ccb->sess_baton = sess_baton;
> ccb->pool = pool;
> + ccb->new_rev = NULL;
> ccb->callback = callback;
> ccb->callback_baton = callback_baton;
We pretty commonly use apr_pcalloc() for exactly this reason. Yeah, it may
cost an extra couple of nanoseconds, but...
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1394346 - /subversion/trunk/subversion/libsvn_ra_svn/client.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Oct 5, 2012 at 4:03 AM, C. Michael Pilato <cm...@collab.net>wrote:
> On 10/04/2012 09:55 PM, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Fri Oct 5 01:54:59 2012
> > New Revision: 1394346
> >
> > URL: http://svn.apache.org/viewvc?rev=1394346&view=rev
> > Log:
> > * subversion/libsvn_ra_svn/client.c
> > (ra_svn_commit): initialize *all* members of the ccb struct
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_ra_svn/client.c
> >
> > Modified: subversion/trunk/subversion/libsvn_ra_svn/client.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/client.c?rev=1394346&r1=1394345&r2=1394346&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_ra_svn/client.c (original)
> > +++ subversion/trunk/subversion/libsvn_ra_svn/client.c Fri Oct 5
> 01:54:59 2012
> > @@ -1044,6 +1044,7 @@ static svn_error_t *ra_svn_commit(svn_ra
> > ccb = apr_palloc(pool, sizeof(*ccb));
> > ccb->sess_baton = sess_baton;
> > ccb->pool = pool;
> > + ccb->new_rev = NULL;
> > ccb->callback = callback;
> > ccb->callback_baton = callback_baton;
>
> We pretty commonly use apr_pcalloc() for exactly this reason.
I'm, aware of that and my original patch had the apr_pcalloc in it.
> Yeah, it may
> cost an extra couple of nanoseconds, but...
>
It's not about the costs. Looking for the root cause of a crash,
I scanned the whole ra_svn -related code for apr_palloc and
found it was used for all baton structures, followed by an
explicit initialization of all elements.
So, I had the option to either implicitly or explicitly set the NULL
pointer and both seemed equally valid.
I went with the explicit assignment.
-- Stefan^2.
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*
Re: svn commit: r1394346 - /subversion/trunk/subversion/libsvn_ra_svn/client.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Oct 5, 2012 at 4:03 AM, C. Michael Pilato <cm...@collab.net>wrote:
> On 10/04/2012 09:55 PM, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Fri Oct 5 01:54:59 2012
> > New Revision: 1394346
> >
> > URL: http://svn.apache.org/viewvc?rev=1394346&view=rev
> > Log:
> > * subversion/libsvn_ra_svn/client.c
> > (ra_svn_commit): initialize *all* members of the ccb struct
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_ra_svn/client.c
> >
> > Modified: subversion/trunk/subversion/libsvn_ra_svn/client.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/client.c?rev=1394346&r1=1394345&r2=1394346&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_ra_svn/client.c (original)
> > +++ subversion/trunk/subversion/libsvn_ra_svn/client.c Fri Oct 5
> 01:54:59 2012
> > @@ -1044,6 +1044,7 @@ static svn_error_t *ra_svn_commit(svn_ra
> > ccb = apr_palloc(pool, sizeof(*ccb));
> > ccb->sess_baton = sess_baton;
> > ccb->pool = pool;
> > + ccb->new_rev = NULL;
> > ccb->callback = callback;
> > ccb->callback_baton = callback_baton;
>
> We pretty commonly use apr_pcalloc() for exactly this reason.
I'm, aware of that and my original patch had the apr_pcalloc in it.
> Yeah, it may
> cost an extra couple of nanoseconds, but...
>
It's not about the costs. Looking for the root cause of a crash,
I scanned the whole ra_svn -related code for apr_palloc and
found it was used for all baton structures, followed by an
explicit initialization of all elements.
So, I had the option to either implicitly or explicitly set the NULL
pointer and both seemed equally valid.
I went with the explicit assignment.
-- Stefan^2.
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*