On Tue, Feb 07, 2006 at 01:43:12PM -0500, Daniel Berlin wrote:
> > > [error calling compress2()]
> >
> > Is SVN_ERR_SVNDIFF_INVALID_COMPRESSED_DATA an appropriate error return
> > here, or should we create an additional code?
>
> Ideally, we would transform the error return from zlib into something we
> can understand, which would require a bunch of codes. However, i don't
> believe compression really ever fails unless it runs out of memory, and
> because of it's bounded memory requirements, this should pretty much
> never happen.
>
> So i more or less punted, since this error should occur pretty much
> never. :)
>
Fair enough. The specific error message is still appropriate, even
if the generic one is a little confusing. But like you say, it should
never happen.
> > Presumably that condition should be '(endlen >= in->len)', so that
> > zlib_decode() can unconditionally determine whether the data is compressed
> > or not?
>
> correct.
>
Fixed in r18378.
> >
> > > static svn_error_t *
> > > window_handler (svn_txdelta_window_t *window, void *baton)
> >
> > The original code seems to ensure that the temporary subpool is destroyed
> > in the event of an error, while the new code doesn't. I'm not sure
> > whether this is particularly important though, especially since errors
> > at this point aren't particularly likely.
>
> I remember discussing this with someone when the branch was in
> development. The answer I got was "but if it error'd out here, you are
> going to get all the way back up the chain anyway, and something will
> destroy the parent pool, so it shouldn't matter".
>
Yep, that's what I was thinking. We use a subpool here to store the
temporary data from decoding each window, and it's appropriate that
we should destroy it when we're finished, if possible (the normal
iterator-pool pattern can't be used here, because the callback doesn't
take a pool argument).
But there's no reason that we need to destroy the pool on error: that's
just overcomplicating things.
> > > @@ -650,7 +810,8 @@
> > >
> > > /* Read a window header from STREAM and check it for integer overflow. */
> > > static svn_error_t *
> > > -read_window_header (svn_stream_t *stream, svn_filesize_t *sview_offset,
> > > +read_window_header (svn_stream_t *stream, unsigned int version,
> > > + svn_filesize_t *sview_offset,
> > > apr_size_t *sview_len, apr_size_t *tview_len,
> > > apr_size_t *inslen, apr_size_t *newlen)
> > > {
> >
> > Why add the version argument? It's not used anywhere, so it just bulks
> > out the diff.
>
> The version argument was originally there so we could issue an error if
> the version number was higher than we knew how to handle. I later
> removed the code to do that, and forgot to remove the argument.
>
> If we want that error, i'm happy to readd it, if not, we should
> certainly remove the argument (which should be int svndiff_version
> anyway).
>
Uh, wouldn't the right place to do that be in write_handler(), just
after we read the SVNx header? (and which is where we would previously
have raised the error).
I think that we should check for a supported version, and I'm happy
to revert the changes to read_window_header() and add the check to
write_handler(), if you agree that that's okay.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Feb 8 14:00:05 2006