On Sun, Sep 29, 2002 at 11:35:18PM +0100, Philip Martin wrote:
> 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
>
Eek!
> > + "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.
Fixed.
>
> > + }
> > +
> > + *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?
>
Good idea.
> > + 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.
Right.
>
> > + total += len;
> > + }
>
> Why not use apr_file_read_full and eliminate the loop altogether?
>
Even better idea.
> --
> Philip Martin
>
Thanks, take a look at revision 3254.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 30 22:11:11 2002