Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove COMPAT_FREEBSD4/5/6/7/9 from MINIMAL and FIRECRACKER kernel configurations #1228

Merged
merged 1 commit into from May 23, 2024

Conversation

hhartzer
Copy link
Contributor

@hhartzer hhartzer commented May 10, 2024

PR: 231768

@bsdimp
Copy link
Member

bsdimp commented May 10, 2024

left a comment in the bug, but basically "think it's a great idea, but we should run this by arch@, I don't think anybody will object. Proactively state this won't affect rust and it should be smooth sailing"

@bsdimp
Copy link
Member

bsdimp commented May 10, 2024

In an ideal world, we'd have the compat stuff be an autoload, but there's a few places where it's not just wrapping the older ABI to call the new APIs.

Copy link
Contributor

@ehem ehem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MINIMAL configuration for i386 also features compatibility back to FreeBSD 4.

PowerPC also features compatibility back to FreeBSD 6 in its GENERIC, GENERIC64 and MPC85XXSPE configurations.

Overall seems reasonable to me. Just need to make sure not to miss anything.

@hhartzer hhartzer force-pushed the disable-compat-freebsd-4-5-6-7-9 branch from 0c40e28 to 576cf6b Compare May 10, 2024 23:42
@hhartzer
Copy link
Contributor Author

Good catch on those ones I missed. The old Phabricator review had a couple others as well.

Should be addressed now.

I've emailed arch@ about this topic.

Copy link
Contributor

@ehem ehem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. Concerns about things using older ABIs being less likely to have been checked for security issues is a significant concern.

@bsdimp
Copy link
Member

bsdimp commented May 14, 2024

My sense of the arch@ thread is that there's not a lot of support for this.

@hhartzer
Copy link
Contributor Author

Yeah, I think you're right. I probably shouldn't have mentioned deleting these, although I'm really surprised there's such pushback on this. This isn't something like C89, C99, POSIX, etc. Just very end-of-lifed FreeBSD ABI support.

Do you think the changes in MINIMAL might be acceptable? And possibly FIRECRACKER?

@brooksdavis
Copy link
Contributor

It's worth talking to @cperciva about FIRECRACKER, but it's hard to imagine that any compat options beyond those required for rust/go support are needed there. Likewise I think removing from MINIMAL makes sense.

@emaste
Copy link
Member

emaste commented May 15, 2024

It's worth talking to @cperciva about FIRECRACKER, but it's hard to imagine that any compat options beyond those required for rust/go support are needed there. Likewise I think removing from MINIMAL makes sense.

Rust is 12+ rust-lang/rust#89058
I'm not sure about Go but I think it's the same

@cperciva
Copy link
Contributor

I'm ok with removing these from the FIRECRACKER kernel.

@bsdimp bsdimp self-assigned this May 16, 2024
@bsdimp
Copy link
Member

bsdimp commented May 16, 2024

It's my read of the arch@ thread that there's no community support for removing these from GENERIC, so please drop that part of this change.

@hhartzer hhartzer force-pushed the disable-compat-freebsd-4-5-6-7-9 branch from 576cf6b to 69e6e05 Compare May 16, 2024 18:42
@hhartzer hhartzer changed the title Disable COMPAT_FREEBSD4/5/6/7/9 as default kernel options Remove COMPAT_FREEBSD4/5/6/7/9 from MINIMAL and FIRECRACKER kernel configurations May 16, 2024
@hhartzer
Copy link
Contributor Author

Should be ready for re-review.

…nfigurations

FIRECRACKER is not a legacy config, so remove the really old FreeBSD
versions from it. MINIMAL has a similar history, and limited target
audience which has little to no overlap with really old binaries. Either
of these is really easy to get additional binary compat with the include
directive, so balance things better. Leave GENERIC alone.

PR: 231768
Signed-off-by: Henrich Hartzer <henrichhartzer@tuta.io>
Reviewed by: imp (MINIMAL), cperciva (FIRECRACKER)
Pull Request: freebsd#1228
@bsdimp bsdimp force-pushed the disable-compat-freebsd-4-5-6-7-9 branch from 69e6e05 to 87bf0aa Compare May 23, 2024 20:34
@freebsd-git freebsd-git merged commit 87bf0aa into freebsd:main May 23, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants