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
--=_MailMate_5CA74BE6-CED5-4A7E-8982-9DB900BE9221_= Content-Type: text/plain; charset=UTF-8; format=flowed; markup=markdown Content-Transfer-Encoding: quoted-printable 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=3D7e7f88001d7dfec83cd7568369= be6a587d4a51ff >>>> >>>> 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 =3D=3D 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=E2=80=99t intended to be an ABI change, hence the commit messa= ge. = >> It >> appears that=E2=80=99s 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 =E2=80=9CWe define __time_t as uint64_t if __LP64__, other= wise = uint32_t, and only define __LP64__ if long is 64 bits. In other words: __time_t =3D=3D long.=E2=80=9D bit was me, and that was c= orrect = for x86, but not for other machines. That=E2=80=99s what I got wrong. >> So we=E2=80=99re only talking about armv7 and ppc32, if I=E2=80=99m no= t = >> forgetting >> anything. The former is on the removal list already, and the latter = >> .. >> well, I don=E2=80=99t know how many users there are. Both are likely t= o 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=E2=80=99m 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=E2=80=99m 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=E2=80=99t many but still a few that use the ioctl interface (t= hings = like pftop and snort). I don=E2=80=99t know offhand if they actually use any of the affected cal= ls = though. I could also revert this now and deal with it when I get around to = converting the relevant ioctl calls to netlink. That=E2=80=99s ongoing an= d = 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=E2=80=99s a breaking change anyway. > I agree with Justin that this is not something to MFC. > There=E2=80=99s no plan to MFC this (or any of the other recent pf work, = for = that matter). Best regards, Kristof --=_MailMate_5CA74BE6-CED5-4A7E-8982-9DB900BE9221_= Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable <!DOCTYPE html> <html> <head> <meta http-equiv=3D"Content-Type" content=3D"text/xhtml; charset=3Dutf-8"= > </head> <body><div style=3D"font-family: sans-serif;"><div class=3D"markdown" sty= le=3D"white-space: normal;"> <p dir=3D"auto">On 17 Feb 2025, at 21:22, John Baldwin wrote:</p> <blockquote style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px= solid #136BCE; color: #136BCE;"> <p dir=3D"auto">On 2/17/25 12:08, Kristof Provost wrote:</p> <blockquote style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px= solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"> <p dir=3D"auto">On 17 Feb 2025, at 16:24, John Baldwin wrote:</p> <blockquote style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px= solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"> <p dir=3D"auto">On 2/14/25 12:50, Kristof Provost wrote:</p> <blockquote style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px= solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"> <p dir=3D"auto">The branch main has been updated by kp:</p> <p dir=3D"auto">URL:<br> <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D7e7f88001d7dfec83cd7= 568369be6a587d4a51ff">https://cgit.FreeBSD.org/src/commit/?id=3D7e7f88001= d7dfec83cd7568369be6a587d4a51ff</a></p> <p dir=3D"auto">commit 7e7f88001d7dfec83cd7568369be6a587d4a51ff<br> Author: Kristof Provost <a href=3D"mailto:kp@FreeBSD.org">kp@FreeBSD.= org</a><br> AuthorDate: 2025-02-07 10:29:26 +0000<br> Commit: Kristof Provost <a href=3D"mailto:kp@FreeBSD.org">kp@FreeBSD.= org</a><br> CommitDate: 2025-02-14 17:47:52 +0000</p> <pre style=3D"margin-left: 15px; margin-right: 15px; padding: 5px; border= : thin solid gray; overflow-x: auto; max-width: 90vw; background-color: #= E4E4E4;"><code style=3D"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=3D"auto">uint32_t,<br> and only define <strong>LP64</strong> if long is 64 bits.<br> In other words: __time_t =3D=3D long.<br> ok henning@ deraadt@<br> Obtained from: OpenBSD, guenther <a href=3D"mailto:guenther@openbsd.org"= >guenther@openbsd.org</a>,<br> 6c1b69a0ff<br> Sponsored by: Rubicon Communications, LLC ("Netgate")<br> Differential Revision: <a href=3D"https://reviews.freebsd.org/D48963">ht= tps://reviews.freebsd.org/D48963</a></p> </blockquote> <p dir=3D"auto">This is an ABI change on non-i386 32-bit platforms in Fre= eBSD 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=3D"auto">It wasn=E2=80=99t intended to be an ABI change, hence the= commit message. It<br> appears that=E2=80=99s only correct for x86 though.</p> </blockquote> <p dir=3D"auto">I assumed the commit message was from OpenBSD as the comm= ents about<br> defining time_t conditional on <strong>LP64</strong> are not correct on F= reeBSD<br> (each arch defines a __time_t in <machine/_types.h>, though amd64<b= r> and i386 share x86/include/_types.h which does use an #ifdef that<br> perhaps is the source of confusion?)</p> </blockquote> <p dir=3D"auto">Partially. The =E2=80=9CWe 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 =3D=3D long.=E2=80=9D bit was me, and that was c= orrect for x86, but not for other machines.</p> <p dir=3D"auto">That=E2=80=99s what I got wrong.</p> <blockquote style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px= solid #136BCE; color: #136BCE;"> <blockquote style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px= solid #136BCE; border-left-color: #4B89CF; color: #4B89CF;"> <p dir=3D"auto">So we=E2=80=99re only talking about armv7 and ppc32, if I= =E2=80=99m not forgetting<br> anything. The former is on the removal list already, and the latter ..<br= > well, I don=E2=80=99t know how many users there are. Both are likely to b= e<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=E2= =80=99m 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=E2=80=99m not inclined to do anything more<br= > involved though.</p> <p dir=3D"auto">Do you have any thoughts?</p> </blockquote> <p dir=3D"auto">To be clear, armv7 is planned to be around a bit longer t= han 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 struct= ures<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 t= o)</p> </blockquote> <p dir=3D"auto">This affects ioctl calls, so it can and probably does aff= ect ports. There aren=E2=80=99t many but still a few that use the ioctl i= nterface (things like pftop and snort).<br> I don=E2=80=99t know offhand if they actually use any of the affected cal= ls though.</p> <p dir=3D"auto">I could also revert this now and deal with it when I get = around to converting the relevant ioctl calls to netlink. That=E2=80=99s = ongoing and still aspirationally (but getting less likely) to be complete= d before we branch 15. That may be a better point to make this change, be= cause once the netlink conversion is complete the next major release will= remove the entire ioctl interface, and that=E2=80=99s a breaking change = anyway.</p> <blockquote style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px= solid #136BCE; color: #136BCE;"> <p dir=3D"auto">I agree with Justin that this is not something to MFC.</p= > </blockquote> <p dir=3D"auto">There=E2=80=99s no plan to MFC this (or any of the other = recent pf work, for that matter).</p> <p dir=3D"auto">Best regards,<br> Kristof</p> </div> </div> </body> </html> --=_MailMate_5CA74BE6-CED5-4A7E-8982-9DB900BE9221_=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BA86195D-E0BC-496A-BEE1-95C0AEAA372F>