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/09/07 18:31:05 UTC
Re: svn commit: r1166247 - in
/subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs: fs.h
fs_fs.c
On Wednesday, September 07, 2011 4:16 PM, stsp@apache.org wrote:
> + /* Create successor-ID data for revision zero. */
> SVN_ERR(svn_io_file_open(&file, path_successor_ids(fs, 0, pool),
> APR_WRITE | APR_BUFFERED | APR_CREATE,
> APR_OS_DEFAULT, pool));
> + /* Write the new index. */
> + memset(new_index, 0, sizeof(new_index));
> + n = (apr_int32_t*)&new_index[3];
> + *n = htonl(FSFS_SUCCESSORS_INDEX_SIZE);
That doesn't look right. You're assuming this trick sets new_index[0..3], but can't it set new_index[3..7]?
I can think of several ways to do this that don't run into this bug (using a union, using inline code to get the individual bytes of FSFS_SUCCESSORS_INDEX_SIZE, or even write_full(&int), write_full(new_index + 8, sizeof(new_index)-8)). Of these three one is probably buggy, but at least one ought to be correct.
> + SVN_ERR(svn_io_file_write_full(file, new_index, sizeof(new_index), NULL,
> + pool));
> + /* No successors were created in revision zero. */
> SVN_ERR(svn_io_file_write_full(file, FSFS_SUCCESSOR_IDS_END_MARKER,
> sizeof(FSFS_SUCCESSOR_IDS_END_MARKER) - 1,
> NULL, pool));
> SVN_ERR(svn_io_file_close(file, pool));
Re: svn commit: r1166247 - in
/subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs: fs.h fs_fs.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 07, 2011 at 06:43:01PM +0200, Stefan Sperling wrote:
> On Wed, Sep 07, 2011 at 07:31:05PM +0300, Daniel Shahaf wrote:
> > On Wednesday, September 07, 2011 4:16 PM, stsp@apache.org wrote:
> > > + /* Create successor-ID data for revision zero. */
> > > SVN_ERR(svn_io_file_open(&file, path_successor_ids(fs, 0, pool),
> > > APR_WRITE | APR_BUFFERED | APR_CREATE,
> > > APR_OS_DEFAULT, pool));
> > > + /* Write the new index. */
> > > + memset(new_index, 0, sizeof(new_index));
> > > + n = (apr_int32_t*)&new_index[3];
> > > + *n = htonl(FSFS_SUCCESSORS_INDEX_SIZE);
> >
> > That doesn't look right. You're assuming this trick sets new_index[0..3], but can't it set new_index[3..7]?
>
> This code must set new_index[3..7].
As discussed on IRC, I had an off-by-one -- it must be [4..7].
This bug should have been fixed, among others, in r1166335. Thanks!
Re: svn commit: r1166247 - in
/subversion/branches/fs-successor-ids/subversion/libsvn_fs_fs: fs.h fs_fs.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 07, 2011 at 07:31:05PM +0300, Daniel Shahaf wrote:
> On Wednesday, September 07, 2011 4:16 PM, stsp@apache.org wrote:
> > + /* Create successor-ID data for revision zero. */
> > SVN_ERR(svn_io_file_open(&file, path_successor_ids(fs, 0, pool),
> > APR_WRITE | APR_BUFFERED | APR_CREATE,
> > APR_OS_DEFAULT, pool));
> > + /* Write the new index. */
> > + memset(new_index, 0, sizeof(new_index));
> > + n = (apr_int32_t*)&new_index[3];
> > + *n = htonl(FSFS_SUCCESSORS_INDEX_SIZE);
>
> That doesn't look right. You're assuming this trick sets new_index[0..3], but can't it set new_index[3..7]?
This code must set new_index[3..7].
Recall that the on-disk format of an offset is essentially two big-endian
32-bit integers, with the most significant integer coming first.
I.e. we want the initial index to look like this:
[ 4 bytes zero | htonl(FSFS_SUCCESSORS_INDEX_SIZE) | zero ... ]
When reading the offset for revision zero, the code reads the first
4 bytes into variable 'n', and the second 4 bytes into variable 'm'.
It then combines both into a 64bit offset in native endianness:
*revision_offset = ((apr_uint64_t)(ntohl(n)) << 32) | ntohl(m);
Note that due to a shortcut in the code reading the index we never
actually read the offset for revision 0 from disk (it is always equal
to FSFS_SUCCESSORS_INDEX_SIZE, after all). But the initial index we
write should be correct regardless.