You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2015/01/15 13:38:14 UTC
svn commit: r1652076 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Author: ivan
Date: Thu Jan 15 12:38:13 2015
New Revision: 1652076
URL: http://svn.apache.org/r1652076
Log:
Fix mostly theoretical data corruption in log-addressing enabled FSFS
repositories when physical-to-logical index will be more than 4 GB on
32-bit platform.
* subversion/libsvn_fs_fs/index.c
(svn_fs_fs__p2l_index_append): Use proper type (apr_uint64_t) for local
variable that holds file size.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/index.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/index.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/index.c?rev=1652076&r1=1652075&r2=1652076&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/index.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/index.c Thu Jan 15 12:38:13 2015
@@ -1980,8 +1980,8 @@ svn_fs_fs__p2l_index_append(svn_checksum
apr_uint64_t last_entry_end = 0;
apr_uint64_t last_page_end = 0;
- apr_size_t last_buffer_size = 0; /* byte offset in the spill buffer at
- the begin of the current revision */
+ apr_uint64_t last_buffer_size = 0; /* byte offset in the spill buffer at
+ the begin of the current revision */
apr_uint64_t file_size = 0;
/* temporary data structures that collect the data which will be moved
Re: svn commit: r1652076 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 15 January 2015 at 16:20, Branko Čibej <br...@wandisco.com> wrote:
> On 15.01.2015 13:38, ivan@apache.org wrote:
>> Author: ivan
>> Date: Thu Jan 15 12:38:13 2015
>> New Revision: 1652076
>>
>> URL: http://svn.apache.org/r1652076
>> Log:
>> Fix mostly theoretical data corruption in log-addressing enabled FSFS
>> repositories when physical-to-logical index will be more than 4 GB on
>> 32-bit platform.
>>
>> * subversion/libsvn_fs_fs/index.c
>> (svn_fs_fs__p2l_index_append): Use proper type (apr_uint64_t) for local
>> variable that holds file size.
>
> How about using svn_filesize_t instead, for clarity. Or apr_off_t which
> should always have the required range.
>
I agree that svn_filesize_t is more appropriate type, but most of FSFS
log-addressing code in index.c uses apr_uint64_t. For example
svn_fs_fs__p2l_index_append:
[[[
apr_uint64_t last_entry_end = 0;
apr_uint64_t last_page_end = 0;
apr_uint64_t last_buffer_size = 0; /* byte offset in the spill buffer at
the begin of the current revision */
apr_uint64_t file_size = 0;
]]]
Packed stream functions (stream_write_encoded() etc) also accepts
apr_uint64_t. So I decided just fix obvious mistake and leave decision
on proper type to people with more knowledge how index.c supposed to
work.
Btw I get several warning when compiling index.c in Windows 32-bit
(there are unrelated to r1652076):
[[[
..\..\..\subversion\libsvn_fs_fs\index.c(1356): warning C4389: '!=' :
signed/unsigned mismatch
..\..\..\subversion\libsvn_fs_fs\index.c(1738): warning C4018: '>=' :
signed/unsigned mismatch
..\..\..\subversion\libsvn_fs_fs\index.c(2496): warning C4018: '<' :
signed/unsigned mismatch
..\..\..\subversion\libsvn_fs_fs\index.c(2769): warning C4018: '>=' :
signed/unsigned mismatch
..\..\..\subversion\libsvn_fs_fs\index.c(2796): warning C4018: '>' :
signed/unsigned mismatch
]]]
--
Ivan Zhakov
Re: svn commit: r1652076 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Branko Čibej <br...@wandisco.com>.
On 19.01.2015 00:59, Stefan Fuhrmann wrote:
>
>
> On Thu, Jan 15, 2015 at 2:20 PM, Branko Čibej <brane@wandisco.com
> <ma...@wandisco.com>> wrote:
>
> On 15.01.2015 13:38, ivan@apache.org <ma...@apache.org> wrote:
> > Author: ivan
> > Date: Thu Jan 15 12:38:13 2015
> > New Revision: 1652076
> >
> > URL: http://svn.apache.org/r1652076
> > Log:
> > Fix mostly theoretical data corruption in log-addressing enabled
> FSFS
> > repositories when physical-to-logical index will be more than 4
> GB on
> > 32-bit platform.
> >
> > * subversion/libsvn_fs_fs/index.c
> > (svn_fs_fs__p2l_index_append): Use proper type (apr_uint64_t)
> for local
> > variable that holds file size.
>
> How about using svn_filesize_t instead, for clarity. Or apr_off_t
> which
> should always have the required range.
>
>
> Any of these would be fine(-ish). With apr_off_t, compilers
> would require an explicit downcast because the svn_filesize_t
> returned by the spill buffer is always an int64. The values are
> guaranteed to be non-negative, which makes conversion to
> unsigned safe.
>
> The code in question is part of the "constructor" that writes
> the index section of some file where the target type is uint64.
> Limitations of the local system will be checked when the index
> gets read again.
>
> So ultimately, we will convert to uint64 before writing to disk.
> Ivan chose to do it immediately and there is no drawback to that.
Yes, his argument (that everything else uses plain apr_uint64_t) is valid.
-- Brane
Re: svn commit: r1652076 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Jan 15, 2015 at 2:20 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 15.01.2015 13:38, ivan@apache.org wrote:
> > Author: ivan
> > Date: Thu Jan 15 12:38:13 2015
> > New Revision: 1652076
> >
> > URL: http://svn.apache.org/r1652076
> > Log:
> > Fix mostly theoretical data corruption in log-addressing enabled FSFS
> > repositories when physical-to-logical index will be more than 4 GB on
> > 32-bit platform.
> >
> > * subversion/libsvn_fs_fs/index.c
> > (svn_fs_fs__p2l_index_append): Use proper type (apr_uint64_t) for local
> > variable that holds file size.
>
> How about using svn_filesize_t instead, for clarity. Or apr_off_t which
> should always have the required range.
>
Any of these would be fine(-ish). With apr_off_t, compilers
would require an explicit downcast because the svn_filesize_t
returned by the spill buffer is always an int64. The values are
guaranteed to be non-negative, which makes conversion to
unsigned safe.
The code in question is part of the "constructor" that writes
the index section of some file where the target type is uint64.
Limitations of the local system will be checked when the index
gets read again.
So ultimately, we will convert to uint64 before writing to disk.
Ivan chose to do it immediately and there is no drawback to that.
-- Stefan^2.
Re: svn commit: r1652076 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Branko Čibej <br...@wandisco.com>.
On 15.01.2015 13:38, ivan@apache.org wrote:
> Author: ivan
> Date: Thu Jan 15 12:38:13 2015
> New Revision: 1652076
>
> URL: http://svn.apache.org/r1652076
> Log:
> Fix mostly theoretical data corruption in log-addressing enabled FSFS
> repositories when physical-to-logical index will be more than 4 GB on
> 32-bit platform.
>
> * subversion/libsvn_fs_fs/index.c
> (svn_fs_fs__p2l_index_append): Use proper type (apr_uint64_t) for local
> variable that holds file size.
How about using svn_filesize_t instead, for clarity. Or apr_off_t which
should always have the required range.
-- Brane