Re: svn commit: r37868 - trunk/subversion/libsvn_subr

On Wed, May 27, 2009 at 4:48 PM, Hyrum K.
Wright<hyrum_wright_at_mail.utexas.edu> wrote:
>
> On May 27, 2009, at 3:17 PM, Paul T. Burba wrote:
>
>> Author: pburba
>> Date: Wed May 27 13:17:53 2009
>> New Revision: 37868
>>
>> Log:
>> Make svn_hash_read2 and svn_hash_read_incremental more memory efficient.
>>
>> * subversion/libsvn_subr/hash.c
>>  (hash_read): Use an iterpool for temporary allocations.
>>
>> Modified:
>>  trunk/subversion/libsvn_subr/hash.c
>>
>> Modified: trunk/subversion/libsvn_subr/hash.c
>> URL:
>> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/hash.c?pathrev=37868&r1=37867&r2=37868
>>
>> ==============================================================================
>> --- trunk/subversion/libsvn_subr/hash.c Wed May 27 12:30:14 2009
>>  (r37867)
>> +++ trunk/subversion/libsvn_subr/hash.c Wed May 27 13:17:53 2009
>>  (r37868)
>> @@ -85,27 +85,37 @@ hash_read(apr_hash_t *hash, svn_stream_t
>>  svn_boolean_t eof;
>>  apr_size_t len, keylen, vallen;
>>  char c, *end, *keybuf, *valbuf;
>> +  svn_error_t *err = SVN_NO_ERROR;
>> +  apr_pool_t *iterpool = svn_pool_create(pool);
>>
>>  while (1)
>>    {
>> +      svn_pool_clear(iterpool);
>> +
>>      /* Read a key length line.  Might be END, though. */
>> -      SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof, pool));
>> +      SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof, iterpool));
>>
>>      /* Check for the end of the hash. */
>>      if ((!terminator && eof && buf->len == 0)
>>          || (terminator && (strcmp(buf->data, terminator) == 0)))
>> -        return SVN_NO_ERROR;
>> +        break;
>>
>>      /* Check for unexpected end of stream */
>>      if (eof)
>> -        return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +        {
>> +          err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +          break;
>> +        }
>>
>>      if ((buf->len >= 3) && (buf->data[0] == 'K') && (buf->data[1] == '
>> '))
>>        {
>>          /* Get the length of the key */
>>          keylen = (size_t) strtoul(buf->data + 2, &end, 10);
>>          if (keylen == (size_t) ULONG_MAX || *end != '\0')
>> -            return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +            {
>> +              err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +              break;
>> +            }
>>
>>          /* Now read that much into a buffer. */
>>          keybuf = apr_palloc(pool, keylen + 1);
>> @@ -116,18 +126,24 @@ hash_read(apr_hash_t *hash, svn_stream_t
>>          len = 1;
>>          SVN_ERR(svn_stream_read(stream, &c, &len));
>>          if (c != '\n')
>> -            return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +            {
>> +              err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +              break;
>> +            }
>>
>>          /* Read a val length line */
>> -          SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof, pool));
>> +          SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof,
>> iterpool));
>>
>>          if ((buf->data[0] == 'V') && (buf->data[1] == ' '))
>>            {
>>              vallen = (size_t) strtoul(buf->data + 2, &end, 10);
>>              if (vallen == (size_t) ULONG_MAX || *end != '\0')
>> -                return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
>> NULL);
>> +                {
>> +                  err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
>> NULL);
>> +                  break;
>> +                }
>>
>> -              valbuf = apr_palloc(pool, vallen + 1);
>> +              valbuf = apr_palloc(iterpool, vallen + 1);
>>              SVN_ERR(svn_stream_read(stream, valbuf, &vallen));
>>              valbuf[vallen] = '\0';
>>
>> @@ -135,14 +151,20 @@ hash_read(apr_hash_t *hash, svn_stream_t
>>              len = 1;
>>              SVN_ERR(svn_stream_read(stream, &c, &len));
>>              if (c != '\n')
>> -                return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
>> NULL);
>> +                {
>> +                  err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
>> NULL);
>> +                  break;
>> +                }
>>
>>              /* Add a new hash entry. */
>>              apr_hash_set(hash, keybuf, keylen,
>>                           svn_string_ncreate(valbuf, vallen, pool));
>>            }
>>          else
>> -            return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +            {
>> +              err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +              break;
>> +            }
>>        }
>>      else if (incremental && (buf->len >= 3)
>>               && (buf->data[0] == 'D') && (buf->data[1] == ' '))
>> @@ -153,7 +175,7 @@ hash_read(apr_hash_t *hash, svn_stream_t
>>            return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>>
>>          /* Now read that much into a buffer. */
>> -          keybuf = apr_palloc(pool, keylen + 1);
>> +          keybuf = apr_palloc(iterpool, keylen + 1);
>>          SVN_ERR(svn_stream_read(stream, keybuf, &keylen));
>>          keybuf[keylen] = '\0';
>>
>> @@ -161,14 +183,23 @@ hash_read(apr_hash_t *hash, svn_stream_t
>>          len = 1;
>>          SVN_ERR(svn_stream_read(stream, &c, &len));
>>          if (c != '\n')
>> -            return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +            {
>> +              err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +              break;
>> +            }
>>
>>          /* Remove this hash entry. */
>>          apr_hash_set(hash, keybuf, keylen, NULL);
>>        }
>>      else
>> -        return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +        {
>> +          err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> +          break;
>> +        }
>>    }
>> +
>> +  svn_pool_destroy(iterpool);
>> +  return err;
>
> Not a big deal, but the above should be:
> return svn_error_return(err)
>
> This ensures we get proper debug tracing even when directly returning
> errors.
>
>> ...
>
> I'm a little unsure why the whole break-on-error dance was needed.  We have
> an pretty established habit of just returning errors from within loops and
> not destroying the loop pool.  Usually, the loop pool isn't very big, and
> it'll get destroyed along with it's parent when the error gets handled.

Hi Hyrum,

Agreed, I was getting a bit out of hand with pool cleanup there.
r37979 returns to previous behavior of returning error from within the
loop.

Thanks,

Paul

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2360947
Received on 2009-06-10 16:55:58 CEST