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

[Greg Stein <gs...@lyra.org>]

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

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

[Ben Collins-Sussman <su...@collab.net>]

Greg Stein <gs...@lyra.org> writes:

> 
> use apr_size_t 
> 
> The cast is unnecessary. keybuf is already a 'char *'
> 
> 
> The cast isn't needed.
> 
> 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:

All of this code was simply ripped out of libsvn_subr/hashdump.c,
which was the -first- code ever written for subversion, about two
years ago. 

IIRC, the code used to read/write hashtables to disk require 'pack'
and 'unpack' routines.  This was done so that any old data-type could
be saved and *resurrected* from the hashdump file.  Kind of like
object serialization, or python's 'pickle' thing.  The 'unpack'
routine takes some (void *) and turns it into a printable bytestring
to put in the file.  The 'pack' routine takes a bytestring read from
the file, and returns a (void *) to be stored as a hash value.

Of course, the only existing pack/unpack routines (at the moment) are
for stringbufs, so it's kind of silly to use this generalization when
reading properties in our dumpstream.  I mean, they're props, so we
*know* they're svn_string_t values.  No need for generalization here.
I'll simplify it.


> > +      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).

<blush>

Fixed, as you suggest.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org