On Wed, Oct 29, 2014 at 9:44 AM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 28.10.2014 14:19, stefan2_at_apache.org wrote:
> > Author: stefan2
> > Date: Tue Oct 28 13:19:30 2014
> > New Revision: 1634875
> >
> > URL: http://svn.apache.org/r1634875
> > Log:
> > Speed up packed revprop access by tuning the manifest file parser.
>
> [...]
>
> > +/* Return the minimum length of any packed revprop file name in
> REVPROPS. */
> > +static apr_size_t
> > +get_min_filename_len(packed_revprops_t *revprops)
> > +{
> > + char number_buffer[SVN_INT64_BUFFER_SIZE];
> > +
> > + /* The revprop filenames have the format <REV>.<COUNT> - with <REV>
> being
> > + * at least the first rev in the shard and <COUNT> having at least one
> > + * digit. Thus, the minimum is 2 + #decimal places in the start rev.
> > + */
> > + return svn__i64toa(number_buffer, revprops->manifest_start) + 2;
> > +}
>
> Are you absolutely sure this is correct? According to the comment, you
> should be returning
>
> strlen(svn_i64toa(...)) + 2
>
svn_i64toa returns the number of non-NUL chars written into the
buffer provided by the caller. There is no memory allocation. An
alternative sequence would look like this:
svn_i64toa(buf, val);
return strlen(buf)+2;
> As it is, you end up allocating buffers that are orders of magnitude
> larger than necessary.
>
The value returned by get_min_filename_len is not used to allocate
some buffer but to skip a number of chars in the input buffer during
parsing. Correctness is checked afterwards by comparing the number
of file names parsed with the number of revisions in the shard; they
must match. So, using a value that is too large here is certain to
make the parser error out.
-- Stefan^2.
Received on 2014-10-29 12:22:20 CET