Date: Mon, 17 Feb 2025 22:02:31 +0100 From: Kristof Provost <kp@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 7e7f88001d7d - main - pf: use time_t for storing time_t values Message-ID: <BA86195D-E0BC-496A-BEE1-95C0AEAA372F@FreeBSD.org> In-Reply-To: <06849674-d51f-4a20-9ddb-687e29ece68e@FreeBSD.org> References: <202502141750.51EHoOFm061342@gitrepo.freebsd.org> <5c019c51-949b-4255-bc44-926ac973a1af@FreeBSD.org> <1B3E8B07-037B-4DA9-A8D7-81F866078A39@FreeBSD.org> <06849674-d51f-4a20-9ddb-687e29ece68e@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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 [-- Attachment #2 --] <!DOCTYPE html> <html> <head> <meta http-equiv="Content-Type" content="text/xhtml; charset=utf-8"> </head> <body><div style="font-family: sans-serif;"><div class="markdown" style="white-space: normal;"> <p dir="auto">On 17 Feb 2025, at 21:22, John Baldwin wrote:</p> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; color: #136BCE;"> <p dir="auto">On 2/17/25 12:08, Kristof Provost wrote:</p> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"> <p dir="auto">On 17 Feb 2025, at 16:24, John Baldwin wrote:</p> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"> <p dir="auto">On 2/14/25 12:50, Kristof Provost wrote:</p> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"> <p dir="auto">The branch main has been updated by kp:</p> <p dir="auto">URL:<br> <a href="https://cgit.FreeBSD.org/src/commit/?id=7e7f88001d7dfec83cd7568369be6a587d4a51ff">https://cgit.FreeBSD.org/src/commit/?id=7e7f88001d7dfec83cd7568369be6a587d4a51ff</a></p>; <p dir="auto">commit 7e7f88001d7dfec83cd7568369be6a587d4a51ff<br> Author: Kristof Provost <a href="mailto:kp@FreeBSD.org">kp@FreeBSD.org</a><br> AuthorDate: 2025-02-07 10:29:26 +0000<br> Commit: Kristof Provost <a href="mailto:kp@FreeBSD.org">kp@FreeBSD.org</a><br> CommitDate: 2025-02-14 17:47:52 +0000</p> <pre style="margin-left: 15px; margin-right: 15px; padding: 5px; border: thin solid gray; overflow-x: auto; max-width: 90vw; background-color: #E4E4E4;"><code style="padding: 0 0.25em; background-color: #E4E4E4;"> 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 </code></pre> <p dir="auto">uint32_t,<br> and only define <strong>LP64</strong> if long is 64 bits.<br> In other words: __time_t == long.<br> ok henning@ deraadt@<br> Obtained from: OpenBSD, guenther <a href="mailto:guenther@openbsd.org">guenther@openbsd.org</a>,<br> 6c1b69a0ff<br> Sponsored by: Rubicon Communications, LLC ("Netgate")<br> Differential Revision: <a href="https://reviews.freebsd.org/D48963">https://reviews.freebsd.org/D48963</a></p>; </blockquote> <p dir="auto">This is an ABI change on non-i386 32-bit platforms in FreeBSD since<br> they<br> all use a 64-bit type for time_t that is not the same size as long.<br> Not<br> sure if the ABI change matters on FreeBSD though?</p> </blockquote> <p dir="auto">It wasn’t intended to be an ABI change, hence the commit message. It<br> appears that’s only correct for x86 though.</p> </blockquote> <p dir="auto">I assumed the commit message was from OpenBSD as the comments about<br> defining time_t conditional on <strong>LP64</strong> are not correct on FreeBSD<br> (each arch defines a __time_t in <machine/_types.h>, though amd64<br> and i386 share x86/include/_types.h which does use an #ifdef that<br> perhaps is the source of confusion?)</p> </blockquote> <p dir="auto">Partially. The “We define __time_t as uint64_t if <strong>LP64</strong>, otherwise uint32_t,<br> and only define <strong>LP64</strong> if long is 64 bits.<br> In other words: __time_t == long.” bit was me, and that was correct for x86, but not for other machines.</p> <p dir="auto">That’s what I got wrong.</p> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; color: #136BCE;"> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"> <p dir="auto">So we’re only talking about armv7 and ppc32, if I’m not forgetting<br> anything. The former is on the removal list already, and the latter ..<br> well, I don’t know how many users there are. Both are likely to be<br> embedded platforms where the ABI change is going to be even less<br> relevant (because it really only matters if the kernel and userspace are<br> not updated together, and these are going to be embedded devices that<br> 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<br> carry this (trivial) diff to OpenBSD forever, or we can ignore the ABI<br> breakage given the above. I’m not inclined to do anything more<br> involved though.</p> <p dir="auto">Do you have any thoughts?</p> </blockquote> <p dir="auto">To be clear, armv7 is planned to be around a bit longer than other 32-bit<br> platforms. That said, 32-bit plaforms are all Tier 2, so an ABI breakage<br> in main is not necessarily the end of the world. Presumably these structures<br> aren't used much in ports but only in base system tools anyway? (That<br> is what my question about the ABI change mattering was trying to allude to)</p> </blockquote> <p dir="auto">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).<br> I don’t know offhand if they actually use any of the affected calls though.</p> <p dir="auto">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.</p> <blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; color: #136BCE;"> <p dir="auto">I agree with Justin that this is not something to MFC.</p> </blockquote> <p dir="auto">There’s no plan to MFC this (or any of the other recent pf work, for that matter).</p> <p dir="auto">Best regards,<br> Kristof</p> </div> </div> </body> </html>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BA86195D-E0BC-496A-BEE1-95C0AEAA372F>