On Sat, Dec 19, 2020 at 01:44:45AM +0100, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> > With the patch below, we keep separate variables for the on-disk and
> > memory-mapped sizes which fixes off_t/size_t confusion in some places.
> > Furthermore, we only attempt to map files which will fit into a size_t.
> 
> pack.mapsize is oddly redundant.
> 
> Whether the file is memory-mapped or not is already given by
> pack.map != NULL.  That can be checked directly, rather than
> pack.mapsize == 0.
> 
> So pack.mapsize is merely a convenient abbreviation for
> (size_t)pack.filesize.  But where do you actually need the cast?
> 
> Why not use pack.filesize throughout, and just check
> pack.filesize < SIZE_MAX before the mmap()?
> 
> Storing the size twice in adjacent variables is weird.

I am trying to get pack.c to compile with -Wsign-compare.
Clang's yelling is quoted below.
We could of course add (size_t) casts to avoid most or all of these.

/home/stsp/src/got/libexec/got-index-pack/../../lib/pack.c:569:13: error:
      comparison of integers of different signs: 'off_t' (aka 'long long') and
      'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
        if (offset >= pack->filesize)
            ~~~~~~ ^  ~~~~~~~~~~~~~~
/home/stsp/src/got/libexec/got-index-pack/../../lib/pack.c:649:21: error:
      comparison of integers of different signs: 'off_t' (aka 'long long') and
      'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
                        if (delta_offset >= pack->filesize)
                            ~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
/home/stsp/src/got/libexec/got-index-pack/../../lib/pack.c:758:24: error:
      comparison of integers of different signs: 'off_t' (aka 'long long') and
      'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
        if (delta_data_offset >= pack->filesize)
            ~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
/home/stsp/src/got/libexec/got-index-pack/../../lib/pack.c:773:18: error:
      comparison of integers of different signs: 'off_t' (aka 'long long') and
      'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
        if (base_offset >= pack->filesize)
            ~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
/home/stsp/src/got/libexec/got-index-pack/../../lib/pack.c:833:18: error:
      comparison of integers of different signs: 'off_t' (aka 'long long') and
      'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
        if (base_offset >= pack->filesize)
            ~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
/home/stsp/src/got/libexec/got-index-pack/../../lib/pack.c:1071:26: error:
      comparison of integers of different signs: 'off_t' (aka 'long long') and
      'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
                        if (delta_data_offset >= pack->filesize) {
                            ~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
/home/stsp/src/got/libexec/got-index-pack/../../lib/pack.c:1332:24: error:
      comparison of integers of different signs: 'off_t' (aka 'long long') and
      'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
                if (obj->pack_offset >= pack->filesize)
                    ~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
/home/stsp/src/got/libexec/got-index-pack/../../lib/pack.c:1363:24: error:
      comparison of integers of different signs: 'off_t' (aka 'long long') and
      'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
                if (obj->pack_offset >= pack->filesize)
                    ~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
8 errors generated.
*** Error 1 in libexec/got-index-pack (<sys.mk>:87 'pack.o')
 
> BTW, I guess strictly speaking pack.map == 0 may be a valid address,
> so I wonder whether MAP_FAILED should be used instead to mark a
> non-mapping.

Good point. I'll look into this.