subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject Re: svn commit: r1634875 - /subversion/trunk/subversion/libsvn_fs_fs/revprops.c
Date Wed, 29 Oct 2014 11:21:49 GMT
On Wed, Oct 29, 2014 at 9:44 AM, Branko ─îibej <brane@wandisco.com> wrote:

> On 28.10.2014 14:19, stefan2@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.

Mime
View raw message