On Sat, Nov 20, 2021 at 12:11 AM Alexander Kanavin <alex.kana...@gmail.com> wrote: > > On Sat, 20 Nov 2021 at 05:13, Érico Nogueira <eric...@disroot.org> wrote: >> >> For what it's worth, most of the time64 support patches that I have seen >> use "%lld" and `long long` as the type for portable representation of >> time, instead of intmax_t, but each should work just as well as the >> other. > > > My original version did use %lld, but Khem tweaked it to use intmax_t. > Perhaps he can comment?
intmax_t is not particularly bounded like long long int and sounded the right choice here say if we have 128bit machines in future. > >> >> Might be worth mentioning in the commit that musl 1.2.0 and above uses >> time64 on all platforms, and that glibc 2.34 has added support for >> time64 (which might be enabled by distro-wide CFLAGS), with possibly >> 2.35 or 2.36 making it enabled by default. > > > Yes, I can add that. > >> >> This is the wrong fix, a temporary variable should be used. When time_t >> is still 32 bits, it means you can accidentally write out of bounds for >> times after 2038 on little endian plaforms; and on big endian platforms, >> it's always writing out of bounds (and *only out of bounds*, before >> 2038, so you can't even access the result of fscanf). yes it seems that way, thanks for catching it.` >> >> Having now taken a second look at the code, I don't think this needs to >> be treated as time_t, anyway? cache_config is a `long`, and it's reading >> a max timeout value, which is unlikely to go beyond the hundreds of >> seconds... Given that the function returns an `int`, I'd even consider >> making `cache_config` an `int` directly. > > > I can fix this as well if Khem confirms. treating int is fine. debuginfod_config_cache() is still passed time_t as second param in few call sites which are assigned to cache_config here, so we will still need to convert/truncate that assignment. > > Thanks for the review. > > Alex
