On Tuesday, November 29, 2011 4:08 PM, "Martin Buck" <mb-tmp-fhoirefvba.ncnpur.bet_at_gromit.dyndns.org> wrote:
> On Tue, Nov 29, 2011 at 04:32:39PM +0200, Daniel Shahaf wrote:
> > Why isn't apr_off_t 64 bits?
>
> Sorry, I meant apr_size_t and that seems to correspond to C89's size_t and
> is not supposed to be used for file sizes. apr_off_t is 64 bits, of course.
>
> > > --- subversion-1.7.1/subversion/libsvn_fs_fs/fs_fs.c.orig 2011-10-19 19:28:55.000000000 +0200
> > > +++ subversion-1.7.1/subversion/libsvn_fs_fs/fs_fs.c 2011-11-25 16:53:56.000000000 +0100
> > > @@ -2575,7 +2575,7 @@
> > >
> > > svn_revnum_t base_revision;
> > > apr_off_t base_offset;
> > > - apr_size_t base_length;
> > > + svn_filesize_t base_length;
> >
> > I'd like both BASE_OFFSET and BASE_LENGTH to be 64 bit types. Would it
> > work to make them both svn_filesize_t?
>
> I guess it would work (on a 32 bit machine, svn_filesize_t and apr_off_t are
> both typedefed to "long long int" anyway), but I wanted to make the types
> the same as those of "offset" and "size" in
> libsvn_fs_fs/fs.h:representation_t where the values ultimately get assigned
> to.
>
> But since this is the first time I've looked at the Subversion sources, I
> can't really recommend anything (besides making sure that base_length is 64
> bits).
>
Okay. Since both svn_filesize_t and apr_off_t are signed 64bits it may
be just a bikeshed which type to use for which struct member. I'll
commit your patch and nominate it for backport to 1.7.3. While at it I'll
also scan the code for similar problems.
> > Are there other instances of this problem in our code? (I'll look
> > myself later, but if you know or can easily find such it'll save me time
> > if you identified them.)
>
> I haven't noticed any other instances, but then, I've not searched for them
> either. All I can say is that with the patch, Subversion seems to handle
> large files just fine (but I've done only very limited testing so far).
>
> Thanks,
> Martin
>
Received on 2011-11-29 16:27:07 CET