You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2002/09/29 22:35:18 UTC
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
Re: svn commit: rev 3248 - trunk/subversion/include trunk/subversion/libsvn_subr
Posted by mark benedetto king <bk...@inquira.com>.
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