Re: svn commit: rev 3248 - trunk/subversion/include trunk/subversion/libsvn_subr

mbk@tigris.org writes:

> +svn_error_t *
> +svn_pipe_receive(svn_pipe_t *pipe,
> + char **data,
> + apr_size_t *length,
> + apr_pool_t *pool)
> +{
> + unsigned int frame_len = 0;
> + unsigned int total = 0;
> + apr_status_t apr_err;
> +
> + while( 1 )
> + {
> + char c;
> + if (apr_file_getc(&c, pipe->read) != APR_SUCCESS)
> + return svn_error_create(apr_err, 0, 0, pool,

apr_err has not been set

> + "pipe: could not read from peer");
> + if (c == ':')
> + break;
> + frame_len = (frame_len * 10) + (c - '0');

This is not very robust. It doesn't produce an error if there are no
digits, it doesn't produce and error if there are non-digits.

> + }
> +
> + *length = frame_len;
> + *data = apr_pcalloc(pool, frame_len);
> +
> + while (total < frame_len)
> + {
> + char buf[BUFSIZ];

Why use a stack buffer? Why not read into data directly?

> + apr_size_t len = sizeof(buf);
> +
> + apr_err = apr_file_read(pipe->read, buf, &len);
> + if (apr_err)
> + {
> + if (!APR_STATUS_IS_EOF(apr_err))
> + return svn_error_create(apr_err, 0, 0, pool,
> + "pipe: could not read from peer");
> + if ((total+len) < frame_len)
> + return svn_error_create(apr_err, 0, 0, pool,
> + "pipe: premature EOF in read");
> + }
> +
> + memcpy(*data+total, buf, len);

Then the memcpy is not needed.

> + total += len;
> + }

Why not use apr_file_read_full and eliminate the loop altogether?

> +
> + return SVN_NO_ERROR;
> +}

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