[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r937098 - /subversion/trunk/subversion/libsvn_subr/subst.c

From: Greg Stein <gstein_at_gmail.com>
Date: Thu, 22 Apr 2010 20:21:51 -0400

Cool! Much better.

On Thu, Apr 22, 2010 at 19:11, <stsp_at_apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_subr/subst.c Thu Apr 22 23:11:48 2010
>...
> +  mt->saved_baton.in_baton = dup_translation_baton(b->in_baton, pool);
> +  mt->saved_baton.out_baton = dup_translation_baton(b->out_baton, pool);
> +  mt->saved_baton.written = b->written;
> +  mt->saved_baton.readbuf = svn_stringbuf_dup(b->readbuf, pool);
> +  mt->saved_baton.readbuf_off = b->readbuf_off;
> +  mt->saved_baton.buf = apr_pmemdup(pool, b->buf, SVN__STREAM_CHUNK_SIZE + 1);

These allocations are all good, since it is the caller's pool being
used for these.

>...
> @@ -1242,23 +1216,21 @@ static svn_error_t *
>  translated_stream_seek(void *baton, svn_stream_mark_t *mark)
>  {
>   struct translated_stream_baton *b = baton;
> -  struct translated_stream_baton *sb;
> -  mark_translated_t *mark_translated = (mark_translated_t *)mark;
> +  mark_translated_t *mt = (mark_translated_t *)mark;
>
>   /* Flush output buffer if necessary. */
>   if (b->written)
>     SVN_ERR(translate_chunk(b->stream, b->out_baton, NULL, 0, b->iterpool));
>
> -  SVN_ERR(svn_stream_seek(b->stream, mark_translated->mark));
> +  SVN_ERR(svn_stream_seek(b->stream, mt->mark));
>
>   /* Restore translation state. */
> -  sb = mark_translated->saved_baton;
> -  b->in_baton = dup_translation_baton(sb->in_baton, b->pool);
> -  b->out_baton = dup_translation_baton(sb->out_baton, b->pool);
> -  b->written = sb->written;
> -  b->readbuf = svn_stringbuf_dup(sb->readbuf, b->pool);
> -  b->readbuf_off = sb->readbuf_off;
> -  b->buf = apr_pmemdup(b->pool, sb->buf, SVN__STREAM_CHUNK_SIZE + 1);
> +  b->in_baton = dup_translation_baton(mt->saved_baton.in_baton, b->pool);
> +  b->out_baton = dup_translation_baton(mt->saved_baton.out_baton, b->pool);
> +  b->written = mt->saved_baton.written;
> +  b->readbuf = svn_stringbuf_dup(mt->saved_baton.readbuf, b->pool);
> +  b->readbuf_off = mt->saved_baton.readbuf_off;
> +  b->buf = apr_pmemdup(b->pool, mt->saved_baton.buf, SVN__STREAM_CHUNK_SIZE + 1);

But these three allocations concern me. Again, if a caller does a
repeated series of mark/seek, then the stream's pool continue to grow
without bound (and the caller has no control over this).

So the question is whether we can remove these additional allocations.

dup_translation_baton is just an alloc + struct copy. Since there are
no more sub-structures to deal with in a "dup", then I believe the
above two statements could simply be:

b->in_baton = mt->saved_baton.in_baton;
b->out_baton = mt->saved_baton.out_baton;

The readbuf allocation should be pretty simple since we know NUL
characters are not present in these streams. Thus, we could do the
following:

svn_stringbuf_set(b->readbuf, mt->saved_baton.readbuf);

If there will be NUL characters, then we could do other things.

We also know that b->buf and mt->saved_baton.buf are BOTH sized at
SVN__STREAM_CHUNK_SIZE + 1. Therefore, we could do a simple copy,
rather than create a new allocation for b->buf:

memcpy(b->buf, mt->saved_baton.buf, SVN__STREAM_CHUNK_SIZE + 1);

It might not be a bad idea to have a symbol for this. Within the
struct definition for translated_stream_baton, we could do something
like:

  /* ... */
  char *buf;
#define SVN__TRANSLATION_BUF_SIZE (SVN__STREAM_CHUNK_SIZE + 1)

And then include a mention in the 'buf' docstring noting its fixed
size, and to use that symbol.

With these changes, then a mark/seek should not increase the size of
the stream's pool. In fact, we can REMOVE the pool from the baton, so
that problems like this won't happen again. There is a reference in
the translation stream's close callback, but the user of the stream
can be left to deal with the lifetime (by clearing the pool passed to
svn_subst_translated_stream).

>...

Cheers,
-g
Received on 2010-04-23 02:22:18 CEST

This is an archived mail posted to the Subversion Dev mailing list.