Re: svn commit: r1683378 - in /subversion/trunk/subversion/libsvn_fs_fs: pack.c revprops.c

On 3 June 2015 at 19:02, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: ivan_at_apache.org [mailto:ivan_at_apache.org]
>> Sent: woensdag 3 juni 2015 17:49
>> To: commits_at_subversion.apache.org
>> Subject: svn commit: r1683378 - in /subversion/trunk/subversion/libsvn_fs_fs:
>> pack.c revprops.c
>>
>> Author: ivan
>> Date: Wed Jun 3 15:48:35 2015
>> New Revision: 1683378
>>
>> URL: http://svn.apache.org/r1683378
>> Log:
>> Prevent a possible FSFS repository corruption with power or network disk
>> failures during 'svnadmin pack'.
>>
>> * subversion/libsvn_fs_fs/pack.c
>> (close_pack_context): Call svn_io_file_flush_to_disk() for pack file.
>> (pack_phys_addressed): Use svn_io_file_open() to open pack and manifest
>> file and call svn_io_file_flush_to_disk() before closing them.
>>
>> * subversion/libsvn_fs_fs/revprops.c
>> (svn_fs_fs__copy_revprops): Use apr_file_t to write pack file and flush
>> changes to disk before close.
>> (svn_fs_fs__pack_revprops_shard): Use svn_io_file_open() to packed revision
>> properties manifest file and call svn_io_file_flush_to_disk()
>> before closing it.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_fs_fs/pack.c
>> subversion/trunk/subversion/libsvn_fs_fs/revprops.c
>>
>
> <snip>
>
>> Modified: subversion/trunk/subversion/libsvn_fs_fs/revprops.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/revprop
>> s.c?rev=1683378&r1=1683377&r2=1683378&view=diff
>> ================================================================
>> ==============
>> --- subversion/trunk/subversion/libsvn_fs_fs/revprops.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_fs/revprops.c Wed Jun 3 15:48:35
>> 2015
>> @@ -1168,7 +1168,6 @@ svn_fs_fs__copy_revprops(const char *pac
>> apr_file_t *pack_file;
>> svn_revnum_t rev;
>> apr_pool_t *iterpool = svn_pool_create(scratch_pool);
>> - svn_stream_t *stream;
>>
>> /* create empty data buffer and a write stream on top of it */
>> svn_stringbuf_t *uncompressed
>> @@ -1192,6 +1191,7 @@ svn_fs_fs__copy_revprops(const char *pac
>> for (rev = start_rev; rev <= end_rev; rev++)
>> {
>> const char *path;
>> + svn_stream_t *stream;
>>
>> svn_pool_clear(iterpool);
>>
>> @@ -1213,9 +1213,10 @@ svn_fs_fs__copy_revprops(const char *pac
>> SVN_ERR(svn__compress(uncompressed, compressed, compression_level));
>>
>> /* write the pack file content to disk */
>> - stream = svn_stream_from_aprfile2(pack_file, FALSE, scratch_pool);
>> - SVN_ERR(svn_stream_write(stream, compressed->data, &compressed->len));
>> - SVN_ERR(svn_stream_close(stream));
>> + SVN_ERR(svn_io_file_write_full(pack_file, compressed->data, compressed-
>> >len,
>> + NULL, scratch_pool));
>> + SVN_ERR(svn_io_file_flush_to_disk(pack_file, scratch_pool));
>> + SVN_ERR(svn_io_file_close(pack_file, scratch_pool));
>
Hi Bert,

Thanks for review.

> Minor optimization: you could have used iterpool as the shortest living scratch pool here,
> like how it is used in the last part of the patch.
Personally I find using iterpool out of loop slightly confusing, so I
prefer don't use such practice unless it's necessary.

-- 
Ivan Zhakov