You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2015/06/02 17:19:26 UTC
svn commit: r1683126 - /subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Author: philip
Date: Tue Jun 2 15:19:26 2015
New Revision: 1683126
URL: http://svn.apache.org/r1683126
Log:
Avoid using pool memory after clear in FSX.
* subversion/libsvn_fs_x/cached_data.c
(read_dir_entries): Copy hashed values into hash pool.
Modified:
subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Modified: subversion/trunk/subversion/libsvn_fs_x/cached_data.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/cached_data.c?rev=1683126&r1=1683125&r2=1683126&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/cached_data.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/cached_data.c Tue Jun 2 15:19:26 2015
@@ -2494,7 +2494,9 @@ read_dir_entries(apr_array_header_t *ent
{
/* We must be in incremental mode */
assert(hash);
- apr_hash_set(hash, entry.key, entry.keylen, NULL);
+ apr_hash_set(hash,
+ apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
+ entry.keylen, NULL);
continue;
}
@@ -2534,7 +2536,9 @@ read_dir_entries(apr_array_header_t *ent
/* In incremental mode, update the hash; otherwise, write to the
* final array. */
if (incremental)
- apr_hash_set(hash, entry.key, entry.keylen, dirent);
+ apr_hash_set(hash,
+ apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
+ entry.keylen, dirent);
else
APR_ARRAY_PUSH(entries, svn_fs_x__dirent_t *) = dirent;
}
Re: svn commit: r1683126 - /subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Posted by Branko Čibej <br...@wandisco.com>.
On 02.06.2015 18:30, Bert Huijben wrote:
> Strange… this should never be necessarily when *removing* something
> from a hash (value =NULL).
>
> I think you see some kind of other problem.
>
> Bert
>
> Sent from Surface
>
> *From:* Philip Martin <ma...@apache.org>
> *Sent:* Tuesday, June 2, 2015 5:19 PM
> *To:* commits@subversion.apache.org <ma...@subversion.apache.org>
>
> Author: philip
> Date: Tue Jun 2 15:19:26 2015
> New Revision: 1683126
>
> URL: http://svn.apache.org/r1683126
> Log:
> Avoid using pool memory after clear in FSX.
>
> * subversion/libsvn_fs_x/cached_data.c
> (read_dir_entries): Copy hashed values into hash pool.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_x/cached_data.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/cached_data.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/cached_data.c?rev=1683126&r1=1683125&r2=1683126&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_x/cached_data.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_x/cached_data.c Tue Jun 2
> 15:19:26 2015
> @@ -2494,7 +2494,9 @@ read_dir_entries(apr_array_header_t *ent
> {
> /* We must be in incremental mode */
> assert(hash);
> - apr_hash_set(hash, entry.key, entry.keylen, NULL);
> + apr_hash_set(hash,
> + apr_pstrmemdup(scratch_pool, entry.key,
> entry.keylen),
> + entry.keylen, NULL);
> continue;
> }
I agree with Bert: this change is unnecessary.
-- Brane
Re: svn commit: r1683126 - /subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> FWIW libsvn_fsfs uses dirent->name as key since it's already copied to
> result_pool:
That was a FSFS change in r1572049; I think it is a better solution so I
have applied it to FSX.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1683126 - /subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 3 June 2015 at 12:39, Philip Martin <ph...@wandisco.com> wrote:
> Bert Huijben <be...@qqmail.nl> writes:
>
>> Strange… this should never be necessarily when *removing* something
>> from a hash (value =NULL).
>>
>>
>> I think you see some kind of other problem.
>
> I agree this bit is unnecessary, I have changed it back.
>
>> @@ -2494,7 +2494,9 @@ read_dir_entries(apr_array_header_t *ent
>> {
>> /* We must be in incremental mode */
>> assert(hash);
>> - apr_hash_set(hash, entry.key, entry.keylen, NULL);
>> + apr_hash_set(hash,
>> + apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
>> + entry.keylen, NULL);
>> continue;
>> }
>>
>
> This bit is the bug fix. It fixes some FAILs seen for fs-tests and
> valgrind identified memory reads after free. I wrote the fix in the
> wrong place first of all and then assumed I needed to fix it in both
> places.
>
>> @@ -2534,7 +2536,9 @@ read_dir_entries(apr_array_header_t *ent
>> /* In incremental mode, update the hash; otherwise, write to the
>> * final array. */
>> if (incremental)
>> - apr_hash_set(hash, entry.key, entry.keylen, dirent);
>> + apr_hash_set(hash,
>> + apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
>> + entry.keylen, dirent);
>> else
>> APR_ARRAY_PUSH(entries, svn_fs_x__dirent_t *) = dirent;
>> }
>
FWIW libsvn_fsfs uses dirent->name as key since it's already copied to
result_pool:
[[[
/* In incremental mode, update the hash; otherwise, write to the
* final array. Be sure to use hash keys that survive this iteration.
*/
if (incremental)
apr_hash_set(hash, dirent->name, entry.keylen, dirent);
else
APR_ARRAY_PUSH(entries, svn_fs_dirent_t *) = dirent;
]]]
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1683126 - /subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Posted by Philip Martin <ph...@wandisco.com>.
Bert Huijben <be...@qqmail.nl> writes:
> Strange… this should never be necessarily when *removing* something
> from a hash (value =NULL).
>
>
> I think you see some kind of other problem.
I agree this bit is unnecessary, I have changed it back.
> @@ -2494,7 +2494,9 @@ read_dir_entries(apr_array_header_t *ent
> {
> /* We must be in incremental mode */
> assert(hash);
> - apr_hash_set(hash, entry.key, entry.keylen, NULL);
> + apr_hash_set(hash,
> + apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
> + entry.keylen, NULL);
> continue;
> }
>
This bit is the bug fix. It fixes some FAILs seen for fs-tests and
valgrind identified memory reads after free. I wrote the fix in the
wrong place first of all and then assumed I needed to fix it in both
places.
> @@ -2534,7 +2536,9 @@ read_dir_entries(apr_array_header_t *ent
> /* In incremental mode, update the hash; otherwise, write to the
> * final array. */
> if (incremental)
> - apr_hash_set(hash, entry.key, entry.keylen, dirent);
> + apr_hash_set(hash,
> + apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
> + entry.keylen, dirent);
> else
> APR_ARRAY_PUSH(entries, svn_fs_x__dirent_t *) = dirent;
> }
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1683126 - /subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Posted by Bert Huijben <be...@qqmail.nl>.
Strange… this should never be necessarily when *removing* something from a hash (value =NULL).
I think you see some kind of other problem.
Bert
Sent from Surface
From: Philip Martin
Sent: Tuesday, June 2, 2015 5:19 PM
To: commits@subversion.apache.org
Author: philip
Date: Tue Jun 2 15:19:26 2015
New Revision: 1683126
URL: http://svn.apache.org/r1683126
Log:
Avoid using pool memory after clear in FSX.
* subversion/libsvn_fs_x/cached_data.c
(read_dir_entries): Copy hashed values into hash pool.
Modified:
subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Modified: subversion/trunk/subversion/libsvn_fs_x/cached_data.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/cached_data.c?rev=1683126&r1=1683125&r2=1683126&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/cached_data.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/cached_data.c Tue Jun 2 15:19:26 2015
@@ -2494,7 +2494,9 @@ read_dir_entries(apr_array_header_t *ent
{
/* We must be in incremental mode */
assert(hash);
- apr_hash_set(hash, entry.key, entry.keylen, NULL);
+ apr_hash_set(hash,
+ apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
+ entry.keylen, NULL);
continue;
}
@@ -2534,7 +2536,9 @@ read_dir_entries(apr_array_header_t *ent
/* In incremental mode, update the hash; otherwise, write to the
* final array. */
if (incremental)
- apr_hash_set(hash, entry.key, entry.keylen, dirent);
+ apr_hash_set(hash,
+ apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
+ entry.keylen, dirent);
else
APR_ARRAY_PUSH(entries, svn_fs_x__dirent_t *) = dirent;
}
Re: svn commit: r1683126 - /subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Posted by Bert Huijben <be...@qqmail.nl>.
Strange… this should never be necessarily when *removing* something from a hash (value =NULL).
I think you see some kind of other problem.
Bert
Sent from Surface
From: Philip Martin
Sent: Tuesday, June 2, 2015 5:19 PM
To: commits@subversion.apache.org
Author: philip
Date: Tue Jun 2 15:19:26 2015
New Revision: 1683126
URL: http://svn.apache.org/r1683126
Log:
Avoid using pool memory after clear in FSX.
* subversion/libsvn_fs_x/cached_data.c
(read_dir_entries): Copy hashed values into hash pool.
Modified:
subversion/trunk/subversion/libsvn_fs_x/cached_data.c
Modified: subversion/trunk/subversion/libsvn_fs_x/cached_data.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/cached_data.c?rev=1683126&r1=1683125&r2=1683126&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/cached_data.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/cached_data.c Tue Jun 2 15:19:26 2015
@@ -2494,7 +2494,9 @@ read_dir_entries(apr_array_header_t *ent
{
/* We must be in incremental mode */
assert(hash);
- apr_hash_set(hash, entry.key, entry.keylen, NULL);
+ apr_hash_set(hash,
+ apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
+ entry.keylen, NULL);
continue;
}
@@ -2534,7 +2536,9 @@ read_dir_entries(apr_array_header_t *ent
/* In incremental mode, update the hash; otherwise, write to the
* final array. */
if (incremental)
- apr_hash_set(hash, entry.key, entry.keylen, dirent);
+ apr_hash_set(hash,
+ apr_pstrmemdup(scratch_pool, entry.key, entry.keylen),
+ entry.keylen, dirent);
else
APR_ARRAY_PUSH(entries, svn_fs_x__dirent_t *) = dirent;
}