You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2014/09/30 14:40:00 UTC
Re: svn commit: r1628393 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
On 30.09.2014 13:10, stefan2@apache.org wrote:
> Author: stefan2
> Date: Tue Sep 30 11:10:02 2014
> New Revision: 1628393
>
> URL: http://svn.apache.org/r1628393
> Log:
> Fix / silence some integer conversion warnings.
>
> * subversion/libsvn_fs_fs/index.c
> (packed_stream_read): We know those are well within apr_size_t's
> value range.
> (auto_open_l2p_index): The config reader limits the BLOCK_SIZE
> to apr_size_t since r1628392.
> (get_l2p_header_body): Use the appropriate type for the iteration
> variable. Use maximally widening casts
> for non-negative values where appropriate.
> Cast revnum count after we verified its range
> to be within MAX_FILES_PER_DIR.
> (prefetch_l2p_pages): Exit early for non-positive ranges and use
> maximally widening casts for non-negative
> values where appropriate.
Uh.
In my book, every cast is wrong, and widening casts are doubly wrong
because they're implicit. Casts are marginally acceptable if they're
needed to adapt to a poorly designed and/or hardware/platform-specific
API. But in this case, all the values involved appear to be parameters
or return values of our own private functions.
-- Brane
Re: svn commit: r1628393 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Branko Čibej <br...@wandisco.com>.
On 30.09.2014 15:39, Stefan Fuhrmann wrote:
> On Tue, Sep 30, 2014 at 2:40 PM, Branko Čibej <brane@wandisco.com
> <ma...@wandisco.com>> wrote:
>
> On 30.09.2014 13:10, stefan2@apache.org
> <ma...@apache.org> wrote:
> > Author: stefan2
> > Date: Tue Sep 30 11:10:02 2014
> > New Revision: 1628393
> >
> > URL: http://svn.apache.org/r1628393
> > Log:
> > Fix / silence some integer conversion warnings.
> >
> > * subversion/libsvn_fs_fs/index.c
> > (packed_stream_read): We know those are well within apr_size_t's
> > value range.
> > (auto_open_l2p_index): The config reader limits the BLOCK_SIZE
> > to apr_size_t since r1628392.
> > (get_l2p_header_body): Use the appropriate type for the iteration
> > variable. Use maximally widening casts
> > for non-negative values where appropriate.
> > Cast revnum count after we verified its
> range
> > to be within MAX_FILES_PER_DIR.
> > (prefetch_l2p_pages): Exit early for non-positive ranges and use
> > maximally widening casts for non-negative
> > values where appropriate.
>
> Uh.
>
> In my book, every cast is wrong,
>
>
> Well, casts are undesirable but not generally wrong.
> I wish I could do with "just int" but that is too large
> for arrays on e.g. x32 and too small for file sizes
> on others, e.g. Windows.
>
> The index code is particularly affected as it needs
> to mediate between in-memory sizes (apr_size_t),
> platform-independent representation (apr_uint64_t)
> and file addressing (apr_off_t).
As I said, it's OK to cast on API boundaries, as long as we can prove
that the casts are safe. SVN<->APR is an API boundary.
>
>
> and widening casts are doubly wrong
> because they're implicit.
>
>
> Yes, they are implicit but VS still warns about signed/
> unsigned conversions. The simplest portable way I can
> think of to make these comparisons work is to cast
> to the largest unsigned we have once we made sure
> that the value is non-negative.
Couldn't care less about VS ... it also warns about a ton of stuff
that's perfectly valid C even when casts are not involved.
The problem is that casts tend to mask actual coding errors from the
compiler. I prefer warnigns from some silly compiler to adding casts
just for the sake of shutting it up.
> Casts are marginally acceptable if they're
> needed to adapt to a poorly designed and/or hardware/platform-specific
> API. But in this case, all the values involved appear to be parameters
> or return values of our own private functions.
>
>
> No. The first one, for instance, converts from apr_off_t
> (for positions in the pack / rev file currently being read)
> to apr_size_t (what the system returns from sizeof).
Red flag right there: what guarantees that a revision or pack file
cannot be larger than 4 gigs, even on 32-bit platforms? One large binary
file (think video assets or chip design files) can easily be larger than
that. I thought we did not have restrictions on file content size.
-- Brane
Re: svn commit: r1628393 - /subversion/trunk/subversion/libsvn_fs_fs/index.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Sep 30, 2014 at 2:40 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 30.09.2014 13:10, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Tue Sep 30 11:10:02 2014
> > New Revision: 1628393
> >
> > URL: http://svn.apache.org/r1628393
> > Log:
> > Fix / silence some integer conversion warnings.
> >
> > * subversion/libsvn_fs_fs/index.c
> > (packed_stream_read): We know those are well within apr_size_t's
> > value range.
> > (auto_open_l2p_index): The config reader limits the BLOCK_SIZE
> > to apr_size_t since r1628392.
> > (get_l2p_header_body): Use the appropriate type for the iteration
> > variable. Use maximally widening casts
> > for non-negative values where appropriate.
> > Cast revnum count after we verified its range
> > to be within MAX_FILES_PER_DIR.
> > (prefetch_l2p_pages): Exit early for non-positive ranges and use
> > maximally widening casts for non-negative
> > values where appropriate.
>
> Uh.
>
> In my book, every cast is wrong,
Well, casts are undesirable but not generally wrong.
I wish I could do with "just int" but that is too large
for arrays on e.g. x32 and too small for file sizes
on others, e.g. Windows.
The index code is particularly affected as it needs
to mediate between in-memory sizes (apr_size_t),
platform-independent representation (apr_uint64_t)
and file addressing (apr_off_t).
> and widening casts are doubly wrong
> because they're implicit.
Yes, they are implicit but VS still warns about signed/
unsigned conversions. The simplest portable way I can
think of to make these comparisons work is to cast
to the largest unsigned we have once we made sure
that the value is non-negative.
> Casts are marginally acceptable if they're
> needed to adapt to a poorly designed and/or hardware/platform-specific
> API. But in this case, all the values involved appear to be parameters
> or return values of our own private functions.
>
No. The first one, for instance, converts from apr_off_t
(for positions in the pack / rev file currently being read)
to apr_size_t (what the system returns from sizeof).
The wiggle room we do have is with the data types
we use in our structs. The options are:
* Use platform-specific types. Then, we may need to
convert at the interfaces (files, network and API).
* Use SVN-specific types of a fixed size. Then we
need to convert when calling into libs / OS.
Traditionally, SVN tends to follow the first approach and
has seen some problems with 'long' as revision number
and 'int' as array index. For some parts of the FSFS index
and more so in FSX, I'm following the second approach.
Not sure which one is actually better in the long run.
-- Stefan^2.