Skip to content

posix: pthread: implement pthread_atfork() #65641

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

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Nov 23, 2023

pthread_atfork() is required by the POSIX_THREADS_BASE Option Group as detailed in Section E.1 of IEEE-1003.1-2017.

The POSIX_THREADS_BASE Option Group is required for PSE51, PSE52, PSE53, and PSE54 conformance, and is otherwise mandatory for any POSIX conforming system as per Section A.2.1.3 of IEEE-1003-1.2017.

Since Zephyr does not yet support processes and (by extension) fork(), this implementation includes a deviation and should be categorized as producing undefined behaviour.

Fixes #59934

@zephyrbot zephyrbot added area: POSIX POSIX API Library area: CMSIS API Layer CMSIS-RTOS API Layer area: Portability Standard compliant code, toolchain abstraction labels Nov 23, 2023
@zephyrbot zephyrbot requested a review from nashif November 23, 2023 02:57
kartben
kartben previously approved these changes Nov 23, 2023
ycsin
ycsin previously approved these changes Nov 23, 2023
pthread_atfork() is required by the POSIX_THREADS_BASE
Option Group as detailed in Section E.1 of IEEE-1003.1-2017.

The POSIX_THREADS_BASE Option Group is required for PSE51,
PSE52, PSE53, and PSE54 conformance, and is otherwise
mandatory for any POSIX conforming system as per Section
A.2.1.3 of IEEE-1003-1.2017.

Since Zephyr does not yet support processes and (by extension)
fork(), this implementation includes a deviation and should be
categorized as producing undefined behaviour.

Signed-off-by: Christopher Friedt <[email protected]>
Mark pthread_atfork() as supported in the documentation.

This option is a mandatory requirement (i.e. it must be present)
even when calling it produces undefined behaviour. That is the
case here, and documentation sbould be updated to reflect that.

Signed-off-by: Christopher Friedt <[email protected]>
Add a simple existence check that pthread_atfork() has
some kind of implementation. The function is mandatory
by all conforming POSIX systems.

Signed-off-by: Christopher Friedt <[email protected]>
@cfriedt cfriedt dismissed stale reviews from ycsin and kartben via 4ef7845 November 23, 2023 03:49
@cfriedt cfriedt force-pushed the issues/59934/posix-implement-pthread-atfork branch from c026c17 to 4ef7845 Compare November 23, 2023 03:49
@cfriedt
Copy link
Member Author

cfriedt commented Nov 23, 2023

@kartben , @ycsin - sorry please re-ack. I just rebased (split some unrelated commits into #65642

@kartben
Copy link
Collaborator

kartben commented Nov 23, 2023

@kartben , @ycsin - sorry please re-ack. I just rebased (split some unrelated commits into #65642

Might be even better to just wait for posix docs change to be merged as this will conflict anyway, right?

@cfriedt cfriedt added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Nov 23, 2023
@cfriedt
Copy link
Member Author

cfriedt commented Nov 23, 2023

Might be even better to just wait for posix docs change to be merged as this will conflict anyway, right?

A fast forward rebase with git managed to work fine. No merge conflict, and the .rst change made it to the right place in the end.

I was impressed

@cfriedt cfriedt merged commit 41b7c17 into zephyrproject-rtos:main Nov 23, 2023
@cfriedt cfriedt deleted the issues/59934/posix-implement-pthread-atfork branch November 23, 2023 05:59
@kartben
Copy link
Collaborator

kartben commented Nov 23, 2023

Please refrain from flagging such trivial-but-not-one-line either changes as trivial, especially if you're the one ending up merging. And trivial requires at least 4 hours in review before being merged anyway :)

@cfriedt
Copy link
Member Author

cfriedt commented Nov 28, 2023

Please refrain from flagging such trivial-but-not-one-line either changes as trivial, especially if you're the one ending up merging. And trivial requires at least 4 hours in review before being merged anyway :)

That's ridiculous. This change was trivial.

If we're going to be so pedantic should I make 4 separate trivial PRs for each of the 4 lines of (directly related) changes in separate files?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: CMSIS API Layer CMSIS-RTOS API Layer area: Portability Standard compliant code, toolchain abstraction area: POSIX POSIX API Library Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

posix: implement pthread_atfork()
4 participants