On 25 June 2014 19:24, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>>
>> On 21 June 2014 17:15, <stefan2_at_apache.org> wrote:
>> > Author: stefan2
>> > Date: Sat Jun 21 15:15:30 2014
>> > New Revision: 1604421
>> >
>> > URL: http://svn.apache.org/r1604421
>> > Log:
>> > Append index data to the rev data file instead of using separate files.
>> >
>> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
>> > space usage as earlier formats. It also eliminates the need for retries
>> > after a potential background pack run because each file is now always
>> > consistent with itself (earlier, data or index files might already have
>> > been deleted while the respective other was still valid). Overall,
>> > most of this patch is removing code necessary to handle separate files.
>> >
>> > The new file format is simple:
>> >
>> > <rep data><l2p index><p2l index><footer><footer length>
>> >
>> > with the first three being identical what we had before. <footer length>
>> > is a single byte giving the length of the preceding footer, so it's
>> > easier to extract than the pre-f7 rev trailer and there is only one
>> > per pack / rev file.
>> >
>> [..]
>>
>>
>> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>>
>> > >==============================================================================
>> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
>> > 2014
>> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >+ "PLAIN\nEND\nENDREP\n"
>> >+ "id: 0.0.r0/2\n"
>> >+ "type: dir\n"
>> >+ "count: 0\n"
>> >+ "text: 0 3 4 4 "
>> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >+ "cpath: /\n"
>> >+ "\n\n"
>> >+
>> >+ /* L2P index */
>> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
>> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
>> > rev */
>> >+ "\6\4" /* page size: bytes, count */
>> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >+
>> >+ /* P2L index */
>> >+ "\0\x6b" /* start rev, rev file size */
>> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> > bytes */
>> >+ "\0" /* offset entry 0 page 1 */
>> >+ /* len, item & type, rev,
>> > checksum */
>> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
>> > */
>> >+
>> >+ /* Footer */
>> >+ "107 121\7",
>> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> This constant makes code unreadable, unmaintainable and very error prone.
>
>
> How?
>
> To make it more obvious that I intend to follow
> the svn_io_file_create_binary interface, I added
> some commentary explaining where the numbers
> come from. But even just placing the sum (without
> its elements) in there would be fine already.
>
> Changing the rev 0 template has never been a fun
> activity and wont become one in the foreseeable
> future.
>
I believe that the committer should be responsible forcommitting
readable and easy to maintain code, not the reviewer. So please fix or
revert.
>>
>>
>> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
>> > URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >
>> > ==============================================================================
>> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
>> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
>> > 15:15:30 2014
>> > @@ -23,6 +23,7 @@
>> > #include "rev_file.h"
>> > #include "fs_fs.h"
>> > #include "index.h"
>> > +#include "low_level.h"
>> > #include "util.h"
>> >
>> > #include "../libsvn_fs/fs-loader.h"
>> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
>> > file->stream = NULL;
>> > file->p2l_stream = NULL;
>> > file->l2p_stream = NULL;
>> > + file->block_size = ffd->block_size;
>> > + file->l2p_offset = -1;
>> > + file->p2l_offset = -1;
>> > + file->footer_offset = -1;
>> > file->pool = pool;
>> > }
>> >
>> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
>> > }
>> >
>> > svn_error_t *
>> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
>> > - svn_fs_t *fs,
>> > - svn_revnum_t rev)
>> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
>> > {
>> > - if (file->file)
>> > - svn_fs_fs__close_revision_file(file);
>> > + if (file->l2p_offset == -1)
>> > + {
>> > + apr_off_t filesize = 0;
>> > + unsigned char footer_length;
>> > + svn_stringbuf_t *footer;
>> > +
>> > + /* Determine file size. */
>> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
>> > file->pool));
>> > +
>> > + /* Read last byte (containing the length of the footer). */
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1, file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
>> > + sizeof(footer_length), NULL, NULL,
>> > + file->pool));
>> > +
>> > + /* Read footer. */
>> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1 - footer_length,
>> > + file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
>> > footer_length,
>> > + &footer->len, NULL, file->pool));
>> You're passing pointer to possible 32-bit value to function accepting
>> pointer to 64-bit value. This will lead unpredictable results.
>
>
> I assume you are referring to the &footer->len?
> In that case, I think I'm passing an apr_size_t*
> to an interface expecting an apr_size_t*.
> What am I missing?
>
You're right. I confused svn_io_file_read_full2() with
svn_io_file_seek(). The code above doesn't have problem I mentioned.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2014-06-25 17:47:03 CEST