Re: apr_off_t is of an ambiguous size.

On Sun, Jan 18, 2004 at 08:53:21AM +0100, Branko ??ibej wrote:
> Not on Windows, where apr_off_t is always 64-bit.

I didn't say anything about Windows. All I was saying is we know there
are platforms with 32-bit apr_off_t's. Nor did I say every platform has
a 32-bit apr_off_t.

> If anyone takes the time to make this a complete patch, please don't
> introduce svn_offset_t. Use the existing svn_filesize_t. Yes, I know
> that's unsigned, but we can fix that (i.e., either make it signed, or
> tell svn_io_file_seek of we're seeking backwards or forwards from the
> current position). But I don't want to see two different types.

What's wrong with two types? We already have two types if we make no
change. It's not like this is really introducing a new type. It's just
replacing apr_off_t with svn_offset_t.

Though, I do see a problem these types the way they are. If filesize is
going to be an unsigned value and the offset type is a signed value then
the offset type has to be twice as big. Otherwise you can't seek to all
points in the file with one operation. Plus APR sets the offset
parameter to your current offset within the file after the operation
completes. Unfortunately it can't do this without overflowing if your
filesize is bigger than your offset type can support.

Alternatives to this are as follows:

* Only have svn_filesize_t as a apr_int64_t. This decreases our
maximum file size to 4GB but provides a consistent way to seek any
location in the file. Use the same type for the offsets.

* svn_filesize_t is an apr_uint32_t and svn_offset_t is an apr_int64_t.
This has the same maximum file size of 4GB.

But the problem here is we don't really gain the ability to seek to all
points in the file anyway. We'd still have these platforms with a
32-bit apr_off_t. There's no way to write (using APR) portable code
that provides that functionality without decreasing our maximum filesize
to 2GB.

Using an unsigned for offsets has its own problems. The diff code
assumes that the offset type is signed. It would likely take more than
trivial changes to make this work. So I don't really see that as a
great option at this point. Plus we're still stuck with a signed
interface to APR and the difficulties with its output into the offset
parameter.

So maybe seeking to all points in a single operation isn't a big deal.
Right now the only thing we seem to be using seek for is to get to the
beginning of the file. In fact svn_io_file_seek isn't being used at
all. So all the seeks are still using arp_off_t even if you applied the
patch as it stands today. And no matter what we do platforms with
32-bit apr_off_t's are not going to be able to support the seeking on
these large files because of the whole offset issue.

So in the end I think the way the patch is implemented now is best. I'd
rather have the additional file length than be able to seek around the
file. No matter what we do we'll still be stuck with the lack of good
cross platform large file support in APR. So this seems like the best
solution until and if APR does that.

Having two types is going to make it a lot easier to deal with whatever
APR does because we'll be able to track down the code effected by such
an APR improvement easier. Mixing the two into one type is going to
force someone to potentially have to go through all the code and figure
out if the type should be changed or not.

I don't think there is a great solution to this problem, just ones with
varying amounts of hassle and annoyance.

-- 
Ben Reser <ben@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org