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

Clean up some workarounds after upgrading minimum kernel version #74519

Closed
5 tasks
tesuji opened this issue Jul 19, 2020 · 9 comments · Fixed by #74606
Closed
5 tasks

Clean up some workarounds after upgrading minimum kernel version #74519

tesuji opened this issue Jul 19, 2020 · 9 comments · Fixed by #74606
Assignees
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@tesuji
Copy link
Contributor

tesuji commented Jul 19, 2020

Besides O_CLOEXEC, which other things would we be able to clean up?

Good question! There are 6 workarounds in libstd for older Linux versions (that I could find).
Increasing the minimum version to 2.6.32 (aka 3rd Kernel LTS, aka RHEL 6) would fix 5 of them.
Code links are inline:

As you can see, the workarounds fixed by this proposal all have a similar flavor.

Originally posted by @josephlr in #62516 (comment)

@tesuji
Copy link
Contributor Author

tesuji commented Jul 19, 2020

Kernel updated in #74163 .

@cuviper
Copy link
Member

cuviper commented Jul 19, 2020

As noted, copy_file_range needs a much newer kernel 4.5, so we'll still need that fallback. But all the CLOEXEC stuff should be ok to assume available now.

@tesuji tesuji changed the title Clean up some workarounds after upgrading minimal kernel version Clean up some workarounds after upgrading minimum kernel version Jul 19, 2020
@jonas-schievink jonas-schievink added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 19, 2020
@RalfJung
Copy link
Member

The new minimal version is 2.6.27 from what I can see, so "Socket::accept uses accept4(2) to permit use of SOCK_CLOEXEC" still needs a fallback, right?

@cuviper
Copy link
Member

cuviper commented Jul 19, 2020

The builder is using 2.6.32 headers, so I would call that our new minimum.

@RalfJung
Copy link
Member

Ah right; I am actually not sure where I got the 27 from...

@cuviper
Copy link
Member

cuviper commented Jul 19, 2020

The title of #62516 suggested 2.6.27, but we went a little higher to match the actual distros in use.

@cuviper
Copy link
Member

cuviper commented Jul 21, 2020

@alexcrichton I know this is ancient history, but do you recall the problem with F_DUPFD_CLOEXEC? You introduced that comment about "some kernel" in 3d28b8b, and you were the last to update it in 0fff73b. If the problem kernels were before 2.6.32, then I'd like to clean that up now.

@cuviper
Copy link
Member

cuviper commented Jul 21, 2020

Just digging in kernel history, I did find one hiccup in torvalds/linux@1219771, but that bug was introduced and resolved during the 3.7 development cycle, not in a release.

@cuviper cuviper self-assigned this Jul 21, 2020
@alexcrichton
Copy link
Member

alexcrichton commented Jul 21, 2020

IIRC we just mirrored what musl did at the time, which looks like it still does:

		int ret = __syscall(SYS_fcntl, fd, F_DUPFD_CLOEXEC, arg);
		if (ret != -EINVAL) {
			if (ret >= 0)
				__syscall(SYS_fcntl, ret, F_SETFD, FD_CLOEXEC);
			return __syscall_ret(ret);
		}

Looks like the original mailing list thread is gone and there's no comments in the musl source, and I don't recall what happened here.

I suspect it would be safe to remove those workarounds and just assume F_DUPFD_CLOEXEC works one the first try until someone says differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants