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

FreeBSDFileStat broken under FreeBSD 12 #126

Closed
Freaky opened this issue Dec 16, 2018 · 20 comments
Closed

FreeBSDFileStat broken under FreeBSD 12 #126

Freaky opened this issue Dec 16, 2018 · 20 comments
Milestone

Comments

@Freaky
Copy link
Contributor

Freaky commented Dec 16, 2018

FreeBSD 12 has a new stat structure, complete with 64-bit inode numbers, link counts, and some field reorderings.

@headius
Copy link
Member

headius commented Dec 17, 2018

We'll need to detect which FreeBSD we're on (or some other way to know which struct to use) and have two definitions, I believe.

@headius headius added this to the 3.0.48 milestone Dec 17, 2018
@Freaky
Copy link
Contributor Author

Freaky commented Dec 18, 2018

I added a simple version check in acc1d9d, which I guess is better than nothing.

A proper fix would, I suppose, involve knowing which ABI we're running under. The above will break in, say, my 11.2 jail env under 12.0, because os.version is still 12.0 :/

@woodsb02
Copy link

The ideal way to detect the ABI would be to read the __FreeBSD_version value.
http://svnweb.freebsd.org/base/head/sys/sys/param.h?view=markup

Values of 1200031 and later have the 64 bit inodes, as shown here:
https://www.freebsd.org/doc/en/books/porters-handbook/versions-12.html

That said, simply detecting it is FreeBSD 12 should be enough for it to work on official releases.

@Freaky
Copy link
Contributor Author

Freaky commented Dec 31, 2018

Does the FFI layer expose symbol information in any way? Could we tell if we've linked to, for instance, lstat@FBSD_1.5?

@ltning
Copy link

ltning commented Jan 2, 2019

This leaves, among other things, Puppet(server) broken on FreeBSD 12. I assume it needs fixing here before a new Puppet version can be rolled; what would it take to fast forward this? The change in acc1d9d seems to match for 12.0 only; anything >=12.0 should be matched (until the next change..)

@Freaky
Copy link
Contributor Author

Freaky commented Jan 2, 2019

It's a lexicographic comparison, anything that sorts after 12.0 should be matched too.

If a version check is to be used, it's probably marginally better to use freebsd-version -u to get the userspace version rather than asking Java for the kernel version.

I suspect the best fix is to add dlvsym() support to jnr-ffi/jffi and explicitly try to load known versioned symbols.

@Freaky
Copy link
Contributor Author

Freaky commented Jan 2, 2019

For prior art, this is basically what Rust's libc crate does:

src/unix/mod.rs
562:    #[cfg_attr(target_os = "freebsd", link_name = "fstat@FBSD_1.0")]
569:    #[cfg_attr(target_os = "freebsd", link_name = "stat@FBSD_1.0")]
603:    #[cfg_attr(target_os = "freebsd", link_name = "readdir@FBSD_1.0")]
608:    #[cfg_attr(target_os = "freebsd", link_name = "readdir_r@FBSD_1.0")]
631:    #[cfg_attr(target_os = "freebsd", link_name = "fstatat@FBSD_1.1")]
786:    #[cfg_attr(target_os = "freebsd", link_name = "lstat@FBSD_1.0")]
972:    #[cfg_attr(target_os = "freebsd", link_name = "mknod@FBSD_1.0")]

src/unix/bsd/mod.rs
462:    #[cfg_attr(target_os = "freebsd", link_name = "glob@FBSD_1.0")]
469:    #[cfg_attr(target_os = "freebsd", link_name = "globfree@FBSD_1.0")]

src/unix/bsd/freebsdlike/mod.rs
1088:    #[cfg_attr(target_os = "freebsd", link_name = "kevent@FBSD_1.0")]
1104:    #[cfg_attr(target_os = "freebsd", link_name = "mknodat@FBSD_1.1")]

Pinning them to to FreeBSD's compatibility shims for now.

@ltning
Copy link

ltning commented Jan 3, 2019

It's a lexicographic comparison, anything that sorts after 12.0 should be matched too.

Yes, sorry, misread that part.

If a version check is to be used, it's probably marginally better to use freebsd-version -u to get the userspace version rather than asking Java for the kernel version.

Not just marginally better, it's absolutely crucial to use the userspace version vs kernel version as long as you're not reading kernel structures directly. Otherwise it'd "mysteriously" break in the case of, say, a 11.2 jail running on a 12.0 kernel - which we're currently doing to keep puppetserver running. Which is, incidentally, also a very common practice "out there".

@Freaky
Copy link
Contributor Author

Freaky commented Jan 3, 2019

I say marginal because I wasn't sure it'd actually work with an 11 package on a 12 userspace, which is also a common practice. Looks like it should, but it's still a fragile-looking band-aid and not what I'd consider a long-term solution.

@smortex
Copy link

smortex commented Jan 3, 2019

I say marginal because I wasn't sure it'd actually work with an 11 package on a 12 userspace, which is also a common practice

This is not supported by FreeBSD. It may happen to work, but we really don't care.

Basically:
  • you can have a kernel newer than the userland (because when you upgrade you want to boot in the new kernel in order to update the userland), but a userland newer than the kernel is not supported;
  • your packages should match the userland. After upgrading, pkg(8) will complain hardly if it detects that the installed packages or for another version of FreeBSD.

This allows you to have jails with a previous version of FreeBSD (userland) with packages for that previous version of FreeBSD. Only the kernel is newer, and that is supported.

@smortex
Copy link

smortex commented Jan 3, 2019

BTW, regarding Java, the .jar file we get at the end should be the same regardless of the environment we build on, right? (I am not used to Java)

So detecting the version of the symbol and using one implementation or the other seems the right way to go, as suggested by @Freaky here #126 (comment)

@Freaky
Copy link
Contributor Author

Freaky commented Jan 3, 2019

This is not supported by FreeBSD. It may happen to work, but we really don't care.

I'd suggest this isn't really a credible position, because it's a major component of almost any system upgrade - you install the new world, reboot, rebuild and reinstall packages, then delete the old libraries they were depending on.

That's a documented process, and it doesn't say "btw all your packages will probably break while you're waiting for it all to rebuild". And it's just as well because it's workflow I've depended on for nearly 20 years.

@Freaky
Copy link
Contributor Author

Freaky commented Mar 18, 2019

Updated against master. Any chance of some movement? Be nice to be able to use JRuby outside a jail again.

@smortex
Copy link

smortex commented Mar 18, 2019

@Freaky I just managed to update this WIP:
jnr/jffi#66

If you can provide feedback / advises / recommendation about the Java part (and also the rest), please do so!

@headius
Copy link
Member

headius commented Apr 9, 2019

Sorry this got left behind a bit...I'll look into mergingnow.

@headius
Copy link
Member

headius commented Apr 9, 2019

@Freaky Is this in a PR somewhere? I'm having trouble finding it.

Once we have a PR we can review and get it merged!

@Freaky
Copy link
Contributor Author

Freaky commented Apr 9, 2019

@headius #132 opened.

@smortex
Copy link

smortex commented Apr 10, 2019

Thanks for submitting this quickfix @Freaky!

In the meantime, I think I reached a point where the right thing is done in all layers of the stack for using versioned symbols:

Reviews and feedback on these pull requests is heavily appreciated: I am not used to Java at all and while this seems to do the right thing™ on my computer, it probably needs some adjustments to match the quality standards one might expect from such projects 😉

@headius
Copy link
Member

headius commented Apr 10, 2019

I'll merge in #132 today. Since various of you are helping us out a lot here, perhaps one or more of you would like to be added as maintainers? We need help!

@headius headius modified the milestones: 3.0.48, 3.0.50 Apr 10, 2019
@headius
Copy link
Member

headius commented Apr 10, 2019

I have filed #134 for us to all collaborate on. It lists links to closed and open BSD-related issues.

I believe this one can be considered resolved with @Freaky's PR merged, correct? Please move general BSD improvement discussion to #134.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants