You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/12/20 14:25:06 UTC

Re: svn commit: r22765 - trunk/subversion/libsvn_client

dlr@tigris.org wrote:
> Modified:
>    trunk/subversion/libsvn_client/copy.c
>
> Modified: trunk/subversion/libsvn_client/copy.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/copy.c?pathrev=22765&r1=22764&r2=22765
> ==============================================================================
> --- trunk/subversion/libsvn_client/copy.c	(original)
> +++ trunk/subversion/libsvn_client/copy.c	Wed Dec 20 06:06:12 2006
>  
>    /* Check that all of our SRCs exist, and all the DSTs don't. */
>    for (i = 0; i < copy_pairs->nelts; i++)
> @@ -221,7 +220,7 @@
>          ((svn_client__copy_pair_t **) (copy_pairs->elts))[i];
>        svn_node_kind_t dst_kind, dst_parent_kind;
>  
> -      svn_pool_clear(subpool);
> +      svn_pool_clear(iterpool);
>  
>        /* Verify that SRC_PATH exists. */
>        SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
>   
Why not this svn_io_check_path from iterpool?



With regards
Kamesh Jayachandran

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r22765 - trunk/subversion/libsvn_client

Posted by Daniel Rall <dl...@collab.net>.
On Wed, 20 Dec 2006, Malcolm Rowe wrote:

> On Wed, Dec 20, 2006 at 07:29:41AM -0800, Daniel Rall wrote:
> > I've tweaked the documentation of svn_io_check_path() in r22771 to
> > note that POOL is only used for temporary allocations (I'm assuming
> > errors fall into this category).
> > 
> 
> Errors are always allocated in their own individual pools, btw.

Yeah, I'm always in rare form when I wake up at 4am after little
sleep.

Re: svn commit: r22765 - trunk/subversion/libsvn_client

Posted by Daniel Rall <dl...@collab.net>.
On Wed, 20 Dec 2006, Kamesh Jayachandran wrote:
...
> >>Given the call it can only modify '&pair->src_kind' as 'pair->src_kind'
> >>is an enum it does not need any allocation, so no need to worry abt pool
> >>destruction.
> >
> >I think Kamesh is right on this one.  The use of the pool parameter
> >isn't documented in svn_io.h, and looking at the internals, pool is only
> >used for allocations inside svn_io_check_path, not for anything that
> >gets returned.  pair->src_kind isn't allocated, just set to an enum value.

I've tweaked the documentation of svn_io_check_path() in r22771 to
note that POOL is only used for temporary allocations (I'm assuming
errors fall into this category).

> Yes we do so the same 9 lines downwards
> 
>      SVN_ERR(svn_io_check_path(pair->dst, &dst_kind, iterpool));

I specifically didn't use POOL here, since we were changing the value
of a local variable.

Re: svn commit: r22765 - trunk/subversion/libsvn_client

Posted by Kamesh Jayachandran <ka...@collab.net>.
>>> As I mentioned in a separate email, this operation allocates memory
>>> for our input parameters.  I want to avoid altering their values, then
>>> destroying the memory holding their new values.  While I don't believe
>>> that those values are used now, this is a safe practice which leaves
>>> the code without booby-traps (and thus easier to modify).
>>>
>>>  - Dan
>>>   
>>>       
>> Given the call it can only modify '&pair->src_kind' as 'pair->src_kind'
>> is an enum it does not need any allocation, so no need to worry abt pool
>> destruction.
>>     
>
> I think Kamesh is right on this one.  The use of the pool parameter
> isn't documented in svn_io.h, and looking at the internals, pool is only
> used for allocations inside svn_io_check_path, not for anything that
> gets returned.  pair->src_kind isn't allocated, just set to an enum value.
>
> Using iterpool here is safe.
>
> -Hyrum
>
>   

Yes we do so the same 9 lines downwards

      SVN_ERR(svn_io_check_path(pair->dst, &dst_kind, iterpool));


With regards
Kamesh Jayachandran

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r22765 - trunk/subversion/libsvn_client

Posted by Daniel Rall <dl...@collab.net>.
On Wed, 20 Dec 2006, Hyrum K. Wright wrote:

> Kamesh Jayachandran wrote:
> > Daniel Rall wrote:
> >> On Wed, 20 Dec 2006, Kamesh Jayachandran wrote:
> >>
> >>  
> >>> dlr@tigris.org wrote:
> >>>    
> >>>> Modified:
> >>>>   trunk/subversion/libsvn_client/copy.c
> >>>>
> >>>> Modified: trunk/subversion/libsvn_client/copy.c
> >>>> URL:
> >>>> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/copy.c?pathrev=22765&r1=22764&r2=22765
> >>>>
> >>>> ==============================================================================
> >>>>
> >>>> --- trunk/subversion/libsvn_client/copy.c    (original)
> >>>> +++ trunk/subversion/libsvn_client/copy.c    Wed Dec 20 06:06:12 2006
> >>>>
> >>>>   /* Check that all of our SRCs exist, and all the DSTs don't. */
> >>>>   for (i = 0; i < copy_pairs->nelts; i++)
> >>>> @@ -221,7 +220,7 @@
> >>>>         ((svn_client__copy_pair_t **) (copy_pairs->elts))[i];
> >>>>       svn_node_kind_t dst_kind, dst_parent_kind;
> >>>>
> >>>> -      svn_pool_clear(subpool);
> >>>> +      svn_pool_clear(iterpool);
> >>>>
> >>>>       /* Verify that SRC_PATH exists. */
> >>>>       SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
> >>>>       
> >>> Why not this svn_io_check_path from iterpool?
> >>>     
> >>
> >> As I mentioned in a separate email, this operation allocates memory
> >> for our input parameters.  I want to avoid altering their values, then
> >> destroying the memory holding their new values.  While I don't believe
> >> that those values are used now, this is a safe practice which leaves
> >> the code without booby-traps (and thus easier to modify).
> >>
> >>  - Dan
> >>   
> > Given the call it can only modify '&pair->src_kind' as 'pair->src_kind'
> > is an enum it does not need any allocation, so no need to worry abt pool
> > destruction.
> 
> I think Kamesh is right on this one.  The use of the pool parameter
> isn't documented in svn_io.h, and looking at the internals, pool is only
> used for allocations inside svn_io_check_path, not for anything that
> gets returned.  pair->src_kind isn't allocated, just set to an enum value.
> 
> Using iterpool here is safe.

Ah, you're right, I wasn't paying close enough attention to the data
type.

Index: copy.c
===================================================================
--- copy.c	(revision 22765)
+++ copy.c	(working copy)
@@ -223,7 +223,7 @@
       svn_pool_clear(iterpool);
 
       /* Verify that SRC_PATH exists. */
-      SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
+      SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, iterpool));
       if (pair->src_kind == svn_node_none)
         return svn_error_createf(SVN_ERR_NODE_UNKNOWN_KIND, NULL,
                                  _("Path '%s' does not exist"),
@@ -238,7 +238,7 @@
                                  _("Path '%s' already exists"),
                                  svn_path_local_style(pair->dst, pool));
 
-      svn_path_split(pair->dst, &pair->dst_parent, &pair->base_name, pool);
+      svn_path_split(pair->dst, &pair->dst_parent, &pair->base_name, iterpool);
 
       /* Make sure the destination parent is a directory and produce a clear
          error message if it is not. */


Any others in the patch?

Re: svn commit: r22765 - trunk/subversion/libsvn_client

Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Kamesh Jayachandran wrote:
> Daniel Rall wrote:
>> On Wed, 20 Dec 2006, Kamesh Jayachandran wrote:
>>
>>  
>>> dlr@tigris.org wrote:
>>>    
>>>> Modified:
>>>>   trunk/subversion/libsvn_client/copy.c
>>>>
>>>> Modified: trunk/subversion/libsvn_client/copy.c
>>>> URL:
>>>> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/copy.c?pathrev=22765&r1=22764&r2=22765
>>>>
>>>> ==============================================================================
>>>>
>>>> --- trunk/subversion/libsvn_client/copy.c    (original)
>>>> +++ trunk/subversion/libsvn_client/copy.c    Wed Dec 20 06:06:12 2006
>>>>
>>>>   /* Check that all of our SRCs exist, and all the DSTs don't. */
>>>>   for (i = 0; i < copy_pairs->nelts; i++)
>>>> @@ -221,7 +220,7 @@
>>>>         ((svn_client__copy_pair_t **) (copy_pairs->elts))[i];
>>>>       svn_node_kind_t dst_kind, dst_parent_kind;
>>>>
>>>> -      svn_pool_clear(subpool);
>>>> +      svn_pool_clear(iterpool);
>>>>
>>>>       /* Verify that SRC_PATH exists. */
>>>>       SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
>>>>       
>>> Why not this svn_io_check_path from iterpool?
>>>     
>>
>> As I mentioned in a separate email, this operation allocates memory
>> for our input parameters.  I want to avoid altering their values, then
>> destroying the memory holding their new values.  While I don't believe
>> that those values are used now, this is a safe practice which leaves
>> the code without booby-traps (and thus easier to modify).
>>
>>  - Dan
>>   
> Given the call it can only modify '&pair->src_kind' as 'pair->src_kind'
> is an enum it does not need any allocation, so no need to worry abt pool
> destruction.

I think Kamesh is right on this one.  The use of the pool parameter
isn't documented in svn_io.h, and looking at the internals, pool is only
used for allocations inside svn_io_check_path, not for anything that
gets returned.  pair->src_kind isn't allocated, just set to an enum value.

Using iterpool here is safe.

-Hyrum

Re: svn commit: r22765 - trunk/subversion/libsvn_client

Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Rall wrote:
> On Wed, 20 Dec 2006, Kamesh Jayachandran wrote:
>
>   
>> dlr@tigris.org wrote:
>>     
>>> Modified:
>>>   trunk/subversion/libsvn_client/copy.c
>>>
>>> Modified: trunk/subversion/libsvn_client/copy.c
>>> URL: 
>>> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/copy.c?pathrev=22765&r1=22764&r2=22765
>>> ==============================================================================
>>> --- trunk/subversion/libsvn_client/copy.c	(original)
>>> +++ trunk/subversion/libsvn_client/copy.c	Wed Dec 20 06:06:12 2006
>>>
>>>   /* Check that all of our SRCs exist, and all the DSTs don't. */
>>>   for (i = 0; i < copy_pairs->nelts; i++)
>>> @@ -221,7 +220,7 @@
>>>         ((svn_client__copy_pair_t **) (copy_pairs->elts))[i];
>>>       svn_node_kind_t dst_kind, dst_parent_kind;
>>>
>>> -      svn_pool_clear(subpool);
>>> +      svn_pool_clear(iterpool);
>>>
>>>       /* Verify that SRC_PATH exists. */
>>>       SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
>>>       
>> Why not this svn_io_check_path from iterpool?
>>     
>
> As I mentioned in a separate email, this operation allocates memory
> for our input parameters.  I want to avoid altering their values, then
> destroying the memory holding their new values.  While I don't believe
> that those values are used now, this is a safe practice which leaves
> the code without booby-traps (and thus easier to modify).
>
>  - Dan
>   
Given the call it can only modify '&pair->src_kind' as 'pair->src_kind' 
is an enum it does not need any allocation, so no need to worry abt pool 
destruction.

With regards
Kamesh Jayachandran

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r22765 - trunk/subversion/libsvn_client

Posted by Daniel Rall <dl...@collab.net>.
On Wed, 20 Dec 2006, Kamesh Jayachandran wrote:

> dlr@tigris.org wrote:
> >Modified:
> >   trunk/subversion/libsvn_client/copy.c
> >
> >Modified: trunk/subversion/libsvn_client/copy.c
> >URL: 
> >http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/copy.c?pathrev=22765&r1=22764&r2=22765
> >==============================================================================
> >--- trunk/subversion/libsvn_client/copy.c	(original)
> >+++ trunk/subversion/libsvn_client/copy.c	Wed Dec 20 06:06:12 2006
> > 
> >   /* Check that all of our SRCs exist, and all the DSTs don't. */
> >   for (i = 0; i < copy_pairs->nelts; i++)
> >@@ -221,7 +220,7 @@
> >         ((svn_client__copy_pair_t **) (copy_pairs->elts))[i];
> >       svn_node_kind_t dst_kind, dst_parent_kind;
> > 
> >-      svn_pool_clear(subpool);
> >+      svn_pool_clear(iterpool);
> > 
> >       /* Verify that SRC_PATH exists. */
> >       SVN_ERR(svn_io_check_path(pair->src, &pair->src_kind, pool));
>
> Why not this svn_io_check_path from iterpool?

As I mentioned in a separate email, this operation allocates memory
for our input parameters.  I want to avoid altering their values, then
destroying the memory holding their new values.  While I don't believe
that those values are used now, this is a safe practice which leaves
the code without booby-traps (and thus easier to modify).

 - Dan