You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/05/16 13:49:52 UTC
Re: svn commit: r1103413 -
/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
stefan2@apache.org wrote on Sun, May 15, 2011 at 14:52:22 -0000:
> Author: stefan2
> Date: Sun May 15 14:52:22 2011
> New Revision: 1103413
>
> URL: http://svn.apache.org/viewvc?rev=1103413&view=rev
> Log:
> If an in-place modification of some cache entry failed, we must remove
> that entry because it might have become invalid or even corrupted.
>
> * subversion/libsvn_subr/cache-membuffer.c
> (membuffer_cache_set_partial): drop the entry upon modification failure.
>
> Found by: danielsh
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c?rev=1103413&r1=1103412&r2=1103413&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/cache-membuffer.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/cache-membuffer.c Sun May 15 14:52:22 2011
> @@ -1331,36 +1331,47 @@ membuffer_cache_set_partial(svn_membuffe
> */
> err = func(&data, &size, baton, pool);
>
> - /* if modification caused a re-allocation, we need to remove the old
> - * entry and to copy the new data back into cache.
> - */
> - if (data != orig_data)
> + if (err)
> {
> - /* Remove the old entry and try to make space for the new one.
> + /* Something somewhere when wrong while FUNC was modifying the
> + * changed item. Thus, it might have become invalid /corrupted.
> + * We better drop that.
> */
> drop_entry(cache, entry);
> - if ( (cache->data_size / 4 > size)
> - && ensure_data_insertable(cache, size))
> + }
> + else
> + {
> + /* if the modification caused a re-allocation, we need to remove
> + * the old entry and to copy the new data back into cache.
> + */
> + if (!err && (data != orig_data))
The "!err" part is unnecessary.
It seems you could do:
if (err)
else if (data != orig_data)
> {
> - /* Write the new entry.
> + /* Remove the old entry and try to make space for the new one.
> */
> - entry->size = size;
> - entry->offset = cache->current_data;
> - if (size)
> - memcpy(cache->data + entry->offset, data, size);
> -
> - /* Link the entry properly.
> - */
> - insert_entry(cache, entry);
> + drop_entry(cache, entry);
> + if ( (cache->data_size / 4 > size)
> + && ensure_data_insertable(cache, size))
> + {
> + /* Write the new entry.
> + */
> + entry->size = size;
> + entry->offset = cache->current_data;
> + if (size)
> + memcpy(cache->data + entry->offset, data, size);
> +
> + /* Link the entry properly.
> + */
> + insert_entry(cache, entry);
>
> #ifdef DEBUG_CACHE_MEMBUFFER
>
> - /* Remember original content, type and key (hashes)
> - */
> - SVN_ERR(store_content_part(tag, data, size, pool));
> - memcpy(&entry->tag, tag, sizeof(*tag));
> + /* Remember original content, type and key (hashes)
> + */
> + SVN_ERR(store_content_part(tag, data, size, pool));
> + memcpy(&entry->tag, tag, sizeof(*tag));
>
> #endif
> + }
> }
> }
> }
>
>
Re: svn commit: r1103413 - /subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
Posted by Stefan Fuhrmann <eq...@web.de>.
On 16.05.2011 13:49, Daniel Shahaf wrote:
> stefan2@apache.org wrote on Sun, May 15, 2011 at 14:52:22 -0000:
>> Author: stefan2
>> Date: Sun May 15 14:52:22 2011
>> New Revision: 1103413
>>
>> URL: http://svn.apache.org/viewvc?rev=1103413&view=rev
>> Log:
>> If an in-place modification of some cache entry failed, we must remove
>> that entry because it might have become invalid or even corrupted.
>>
>> * subversion/libsvn_subr/cache-membuffer.c
>> (membuffer_cache_set_partial): drop the entry upon modification failure.
>>
>> Found by: danielsh
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
>>
>> Modified: subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c?rev=1103413&r1=1103412&r2=1103413&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_subr/cache-membuffer.c (original)
>> +++ subversion/trunk/subversion/libsvn_subr/cache-membuffer.c Sun May 15 14:52:22 2011
>> @@ -1331,36 +1331,47 @@ membuffer_cache_set_partial(svn_membuffe
>> */
>> err = func(&data,&size, baton, pool);
>>
>> - /* if modification caused a re-allocation, we need to remove the old
>> - * entry and to copy the new data back into cache.
>> - */
>> - if (data != orig_data)
>> + if (err)
>> {
>> - /* Remove the old entry and try to make space for the new one.
>> + /* Something somewhere when wrong while FUNC was modifying the
>> + * changed item. Thus, it might have become invalid /corrupted.
>> + * We better drop that.
>> */
>> drop_entry(cache, entry);
>> - if ( (cache->data_size / 4> size)
>> -&& ensure_data_insertable(cache, size))
>> + }
>> + else
>> + {
>> + /* if the modification caused a re-allocation, we need to remove
>> + * the old entry and to copy the new data back into cache.
>> + */
>> + if (!err&& (data != orig_data))
> The "!err" part is unnecessary.
>
> It seems you could do:
>
> if (err)
> else if (data != orig_data)
That was an artifact from intermediate code. Fixed in r1104319.
Thanks for all the reviews. I kinda start relying on them ;)
-- Stefan^2.