Re: svn commit: rev 1912 - trunk/subversion/include trunk/subversion/libsvn_subr trunk/subversion/libsvn_repos

On Wed, May 08, 2002 at 11:23:36PM -0500, sussman@tigris.org wrote:
>...
> +++ trunk/subversion/libsvn_repos/load.c Wed May 8 23:23:36 2002
>...
> + {
> + apr_size_t numread;
> + char *keybuf;
> + char c;
> +
> + /* Get the length of the key */
> + size_t keylen = (size_t) atoi (buf + 2);

use apr_size_t

> + /* Now read that much into a buffer, + 1 byte for null terminator */
> + keybuf = apr_pcalloc (subpool, keylen + 1);
> + numread = keylen;
> + SVN_ERR (svn_stream_read (stream, keybuf, &numread));
> + bytes_sucked += numread;
> + if (numread != keylen)
> + goto stream_ran_dry;
> + ((char *) keybuf)[keylen] = '\0';

The cast is unnecessary. keybuf is already a 'char *'

>...
> + /* Again, 1 extra byte for the null termination. */
> + char *valbuf = apr_palloc (subpool, vallen + 1);
> + numread = vallen;
> + SVN_ERR (svn_stream_read (stream, valbuf, &numread));
> + bytes_sucked += numread;
> + if (numread != vallen)
> + goto stream_ran_dry;
> + ((char *) valbuf)[vallen] = '\0';

The cast isn't needed.

>...
> + /* Send the val data for packaging... */
> + package = (void *) (*pack_func) (vallen, valbuf, subpool);
> + propstring = (svn_string_t *) package;

Why all the funny casting? pack_func should be declared to return an
'svn_string_t *' since that is what you're expecting from it anyways.

Also note that you're passing svn_pack_bytestring() as a pack_func, but that
returns a stringbuf, not a plain string (Badness!). But I've gotta ask: what
the heck is this "pack function" *for* ? You've got a buffer and length, so
why not just construct an svn_string_t yourself? For example:

  svn_string_t propval;
  
  propval.data = valbuf;
  propval.len = vallen;

  .... set_FOO_property(record_baton, keybuf, &propval);

Given that there is a single caller of parse_content_block(), and that
svn_pack_bytestring is always passed, and that that is inherently wrong, I'd
suggest the simpler route suggested above.

>...
> + if (text_stream != NULL)
> + {
> + char buffer[SVN_STREAM_CHUNK_SIZE];
> + apr_size_t buflen = SVN_STREAM_CHUNK_SIZE;

WOAH! You simply cannot allocate that on the stack. You're looking at over
a 100k(!)

I'd suggest allocating the block once at the start of the parse, and simply
reusing it within the parse_content_block() function. e.g. have some kind a
"parse_ctx" structure which can hold that buffer (and maybe a couple
stringbufs for the key/value buffers which can be sized properly and reused).

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org