Re: git: 7e7f88001d7d - main - pf: use time_t for storing time_t values

On 17 Feb 2025, at 21:22, John Baldwin wrote:
> On 2/17/25 12:08, Kristof Provost wrote:
>> On 17 Feb 2025, at 16:24, John Baldwin wrote:
>>> On 2/14/25 12:50, Kristof Provost wrote:
>>>> The branch main has been updated by kp:
>>>>
>>>> URL:
>>>> https://cgit.FreeBSD.org/src/commit/?id=7e7f88001d7dfec83cd7568369be6a587d4a51ff
>>>>
>>>> commit 7e7f88001d7dfec83cd7568369be6a587d4a51ff
>>>> Author:     Kristof Provost <kp@FreeBSD.org>
>>>> AuthorDate: 2025-02-07 10:29:26 +0000
>>>> Commit:     Kristof Provost <kp@FreeBSD.org>
>>>> CommitDate: 2025-02-14 17:47:52 +0000
>>>>
>>>>       pf: use time_t for storing time_t values
>>>>           No change to the underlying type, so no ABI change.
>>>>           We define __time_t as uint64_t if __LP64__, otherwise
>>>> uint32_t,
>>>>       and only define __LP64__ if long is 64 bits.
>>>>       In other words: __time_t == long.
>>>>           ok henning@ deraadt@
>>>>           Obtained from:  OpenBSD, guenther <guenther@openbsd.org>,
>>>> 6c1b69a0ff
>>>>       Sponsored by:   Rubicon Communications, LLC ("Netgate")
>>>>       Differential Revision:  https://reviews.freebsd.org/D48963
>>>
>>> This is an ABI change on non-i386 32-bit platforms in FreeBSD since
>>> they
>>> all use a 64-bit type for time_t that is not the same size as long.
>>> Not
>>> sure if the ABI change matters on FreeBSD though?
>>>
>> It wasn’t intended to be an ABI change, hence the commit message. 
>> It
>> appears that’s only correct for x86 though.
>
> I assumed the commit message was from OpenBSD as the comments about
> defining time_t conditional on __LP64__ are not correct on FreeBSD
> (each arch defines a __time_t in <machine/_types.h>, though amd64
> and i386 share x86/include/_types.h which does use an #ifdef that
> perhaps is the source of confusion?)
>
Partially. The “We define __time_t as uint64_t if __LP64__, otherwise 
uint32_t,
and only define __LP64__ if long is 64 bits.
In other words: __time_t == long.” bit was me, and that was correct 
for x86, but not for other machines.

That’s what I got wrong.

>> So we’re only talking about armv7 and ppc32, if I’m not 
>> forgetting
>> anything. The former is on the removal list already, and the latter 
>> ..
>> well, I don’t know how many users there are. Both are likely to be
>> embedded platforms where the ABI change is going to be even less
>> relevant (because it really only matters if the kernel and userspace 
>> are
>> not updated together, and these are going to be embedded devices that
>> are far more likely to have everything updated simultaneously).> So 
>> I’m unsure about what to do. I can revert this and we can just
>> carry this (trivial) diff to OpenBSD forever, or we can ignore the 
>> ABI
>> breakage given the above. I’m not inclined to do anything more
>> involved though.
>>
>> Do you have any thoughts?
>
> To be clear, armv7 is planned to be around a bit longer than other 
> 32-bit
> platforms.  That said, 32-bit plaforms are all Tier 2, so an ABI 
> breakage
> in main is not necessarily the end of the world.  Presumably these 
> structures
> aren't used much in ports but only in base system tools anyway?  (That
> is what my question about the ABI change mattering was trying to 
> allude to)
>
This affects ioctl calls, so it can and probably does affect ports. 
There aren’t many but still a few that use the ioctl interface (things 
like pftop and snort).
I don’t know offhand if they actually use any of the affected calls 
though.

I could also revert this now and deal with it when I get around to 
converting the relevant ioctl calls to netlink. That’s ongoing and 
still aspirationally (but getting less likely) to be completed before we 
branch 15. That may be a better point to make this change, because once 
the netlink conversion is complete the next major release will remove 
the entire ioctl interface, and that’s a breaking change anyway.

> I agree with Justin that this is not something to MFC.
>
There’s no plan to MFC this (or any of the other recent pf work, for 
that matter).

Best regards,
Kristof