Forward compatibility for ino64.

[imp]

Summary

Add forward compatibility so that new binaries can run on old kernels. If the new system call from ino64 isn't available on your system, then the old one will be used and the results translated. The stat and statfs families of functions are fully emulated. The getdirentries family uses tricks that may not work on remote filesystems. While not required by policy, in this case it is helpful to our users.

Tested on: 12-current binaries on a 10.3-beta kernel run and return consistent results.

(this isn't 100% the commit message, since differential won't let me cut and paste that verbatim)

[imp]

Trying with magic in git.config file.

[imp]

again

[emaste]

lib/libc/sys/stat.c

51 ↗

(On Diff #29579)

__getosreldate() already internally stores the osreldate in a static int.

[kib]

lib/libc/include/libc_private.h

318 ↗

(On Diff #29579)

Why do you cut upper half-word ?

324 ↗

(On Diff #29579)

Again, why ?

333 ↗

(On Diff #29579)

I do not understand this comment, as well I do not understand why it is not set.

Also, spares and paddings might be left alone, there is no leaks, but st_Xtim_ext must be zeroed.

362 ↗

(On Diff #29579)

I do not think this is useful.

363 ↗

(On Diff #29579)

And this.

lib/libc/sys/fstat.c

62 ↗

(On Diff #29579)

fstat and arguably _fstat must be exported as weak symbols, and it is enough to declare fstat as an alias to _fstat. It is much better than to introduce trivial redirect function, and it would be consistent with what other libc code does.

This note holds for all other trivial wrappers, I do not repeat it.

lib/libc/sys/getdirentries.c

110 ↗

(On Diff #29579)

This leaks oldbuf.

[imp]

lib/libc/include/libc_private.h

318 ↗

(On Diff #29579)

Because I accidentally was reading ostat instead of stat. You're right, shouldn't do this.

324 ↗

(On Diff #29579)

bad...

333 ↗

(On Diff #29579)

Yea, that's true. will correct this.

362 ↗

(On Diff #29579)

Should I just zero this?

363 ↗

(On Diff #29579)

ditto?

lib/libc/sys/fstat.c

62 ↗

(On Diff #29579)

OK. Will do. Thanks for the advice. I wondered about the best way to do that, and I agree with your suggestion.

lib/libc/sys/getdirentries.c

110 ↗

(On Diff #29579)

OK. Will fix.

lib/libc/sys/stat.c

51 ↗

(On Diff #29579)

Yea, But it's not inline. And stat is used a crap-ton.

But perhaps the right thing to do is make __getosreldate() inline....

[emaste]

lib/libc/sys/stat.c

51 ↗

(On Diff #29579)

Yeah, there are a few ways we could do it - I just really want to avoid copying and pasting this everywhere.

[kib]

lib/libc/include/libc_private.h

362 ↗

(On Diff #29579)

I think leaving it alone is good enough, same as above. There is no possible leak since both sf and sf11 point to the same address space.

lib/libc/sys/stat.c

51 ↗

(On Diff #29579)

Cost of the trap to execute a syscall is 100x times of the cost of the local function call. I suggest to not bother with inlining unless you can prove 1% difference in a microtest (and I would not ever bother with measurements).

[imp]

Updated with the various issues raised in the code review: weak
symbols, osreldate, missing fields.

Also, use an even smaller buffer so we get all the files in a given
directory.

We likely still need to ensure we don't walk off the end of dst array
before writing to that element. I'm toying with also playing seek
games that are not strictly allowed, but that I know will work instead
of the tiny buffers, but I'm not sure that would be an improvement.

[kib]

lib/libc/include/libc_private.h

302 ↗

(On Diff #29660)

Can you put new content from the libc_private.h into some new header ? First, I do not want the _WANT_FREEBSD11_STAT be abused, it is a single-purpose symbol. Second, include <string.h> there and duplicate fhandle fwd-decl are too rude.

321 ↗

(On Diff #29660)

Style requires empty line after the { if there is no locals.

lib/libc/sys/fhstat.c

42 ↗

(On Diff #29660)

extern is excessive.

lib/libc/sys/getdirentries.c

44 ↗

(On Diff #29660)

Again, no need in 'extern'.

73 ↗

(On Diff #29660)

printf should be removed.

86 ↗

(On Diff #29660)

You removed the caching of osreldate from other functions, while leaving it for getdirentries() ?

105 ↗

(On Diff #29660)

sacrIfice ?

[imp]

Implement kib's suggestions (hopefully I didn't miss any)

[imp]

Missed a spelling mistake kib@ found.

[imp]

Fix leak I overlooked.

[imp]

OK, with the latest update, I think I've addressed everything there.

lib/libc/include/libc_private.h

302 ↗

(On Diff #29660)

Sure. Lemme know what you think of what I did... Not 100% sure I like it, but I think it's better than spamming libc_private.h.

lib/libc/sys/getdirentries.c

73 ↗

(On Diff #29660)

And added sanity check to break early to avoid overflow and then exit.

[imp]

Add getfsstat. needed for df.

[kib]

Overall, I have only one big issue with this patch, ignoring the small nits I noted inline.

I consider this compat work as optional and done by will of the submitter, not as a precedent enforcing new project policy on forward-incompatible ABI changes. In other words, this change must not establish a new policy of requiring forward-compatibility. We only guarantee backward compat. I would appreciate if this will be stated in the commit message explicitely.

lib/libc/sys/compat-ino64.h

25 ↗

(On Diff #29844)

You need $FreeBSD$ tag somewhere.

lib/libc/sys/fstat.c

35 ↗

(On Diff #29844)

The re-inclusion of sys/stat.h header is not needed, and is in fact dangerous due to _WANT_STAT11. The ordering of the headers is also not recommended, sys/param.h should go first, definitely before compat-ino64.h.

lib/libc/sys/getfsstat.c

59 ↗

(On Diff #29844)

return (-1);

[emaste]

I consider this compat work as optional and done by will of the submitter, not as a precedent enforcing new project policy on forward-incompatible ABI changes.

In my opinion I think we should neither mandate nor discourage forward-compatibility shims.

I might say that shims are not required (i.e., by policy), but are appreciated and ought to be implemented when it is straightforward to do so. We should then also clarify that in the the "Backwards compatibility" section in https://wiki.freebsd.org/AddingSyscalls, which currently states only that they are "recommended" and provides no guidance on when they should be removed again.

[imp]

In D11185#233466, @emaste wrote:

I consider this compat work as optional and done by will of the submitter, not as a precedent enforcing new project policy on forward-incompatible ABI changes.

In my opinion I think we should neither mandate nor discourage forward-compatibility shims.

I think that this is too weak. I think they should be encouraged when there's a big impact to our users, like with ino64. While not required, they are quite beneficial. I did the changes for two reasons: one they were a must have, non-optional item for Netflix. Second, I felt that the disruption of changing stat fell into the 'highly desirable' category. Changing kqueue, on the other hand, is largely a non-issue for people because it's not used in the upgrade path.

I might say that shims are not required (i.e., by policy), but are appreciated and ought to be implemented when it is straightforward to do so. We should then also clarify that in the the "Backwards compatibility" section in https://wiki.freebsd.org/AddingSyscalls, which currently states only that they are "recommended" and provides no guidance on when they should be removed again.

Yes. This is closer to what I'd like to see the project encourage. We as a project has always had a range of advice for all our communal activities: you must do X, you must not do Y, you are allowed to do Z, you're encouraged to do W. Forward compat shims are category W when there's a big impact, but a category Z when the impact is small or limited. The fact that it's disruptive to the upgrade process is what moves it from Z to W. A lot of this wording, btw, came from the early days of the project when there was a lot of conflict between the minimalists and maximalists and both sides wanted to win, but the project's best interests were actually somewhere in the flexible middle.

So I guess this is a mild rebuke of ino64 not having FC from the get go. Since ino64 was a big win otherwise, I tried to keep that out of the commit message, however. If it's still in there, I'm open to softening what I've written. I don't wish to pick a fight here, but I also don't wish to let it go by. I also recognize that there's a diversity of views, and others might not share my disappointment and that's OK. I have no desire to create an absolute straight jacket that you must have FC always by doing this commit, but I also wish to make things better and I have a need for that at work which can fund the project.

I agree that we should refine the adding a system call wiki to reflect some of this nuance.

[imp]

Latest nits from kib@

[imp]

I've updated the summary to be the commit message, modulo the content we put in the commit message that differential barfs on.

[kib]

lib/libc/include/libc_private.h

339 ↗

(On Diff #29862)

This looks mis-indented.

lib/libc/sys/statfs.c

33 ↗

(On Diff #29862)

I think that sys/syscall.h is not needed there. Probably in several more places.

[imp]

lib/libc/include/libc_private.h

339 ↗

(On Diff #29862)

It's fine in my fixed-width font text editor, and has tabs in the right places. Looks like a display error for phab.

lib/libc/sys/statfs.c

33 ↗

(On Diff #29862)

I'll check. Thanks.

[imp]

Remove unused includes, per kib.

[imp]

Rebase to latest tip of the spear...

[imp]

In D11185#233556, @imp wrote:

Rebase to latest tip of the spear...

I plan on committing later today unless there's more comments...

[emaste]

I didn't check in enough detail to give a "reviewed by" but my skim just now found nothing further to raise / comment on.

lib/libc/include/libc_private.h

339 ↗

(On Diff #29862)

Yes I believe phab is still replacing <tab> with 8 spaces :(

[kib]

This looks fine to me, with the note I did in the previous accept action.

For such changes, I usually manually compare versioned export list for both rtld and libc for pre- and post-patched builds. In case of your patch, the thing to check is that the export list did not changed.