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

posix: signal: APIs implementation #60083

Merged
merged 10 commits into from
Jul 18, 2023

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented Jul 6, 2023

@ycsin ycsin requested a review from cfriedt as a code owner July 6, 2023 09:31
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Jul 6, 2023
@ycsin ycsin force-pushed the pr/posix-signal-apis branch 2 times, most recently from a70b15f to 09c4b29 Compare July 6, 2023 10:07
@ycsin ycsin force-pushed the pr/posix-signal-apis branch 3 times, most recently from 3a97035 to d0bb37c Compare July 6, 2023 16:25
tests/posix/common/src/signal.c Outdated Show resolved Hide resolved
@ycsin ycsin force-pushed the pr/posix-signal-apis branch from d0bb37c to b9fcfdf Compare July 6, 2023 16:49
@ycsin ycsin requested review from aescolar and daor-oti as code owners July 6, 2023 16:49
@zephyrbot zephyrbot added area: Portability Standard compliant code, toolchain abstraction area: CMSIS API Layer CMSIS-RTOS API Layer labels Jul 6, 2023
@zephyrbot zephyrbot requested a review from nashif July 6, 2023 16:49
@ycsin ycsin force-pushed the pr/posix-signal-apis branch from b9fcfdf to 26c03ad Compare July 6, 2023 17:00
include/zephyr/posix/signal.h Outdated Show resolved Hide resolved
include/zephyr/posix/signal.h Show resolved Hide resolved
lib/posix/signal.c Outdated Show resolved Hide resolved
@ycsin ycsin force-pushed the pr/posix-signal-apis branch 2 times, most recently from 7357b31 to a3098a2 Compare July 7, 2023 02:25
@zephyrbot zephyrbot added the area: Coding Guidelines Coding guidelines and style label Jul 7, 2023
@ycsin ycsin force-pushed the pr/posix-signal-apis branch 2 times, most recently from 64d3496 to 17c8337 Compare July 7, 2023 06:32
include/zephyr/posix/signal.h Outdated Show resolved Hide resolved
include/zephyr/posix/signal.h Outdated Show resolved Hide resolved
include/zephyr/posix/signal.h Outdated Show resolved Hide resolved
include/zephyr/posix/signal.h Show resolved Hide resolved
lib/posix/strsignal.c Outdated Show resolved Hide resolved
lib/posix/signal.c Outdated Show resolved Hide resolved
lib/posix/signal.c Outdated Show resolved Hide resolved
lib/posix/signal.h Outdated Show resolved Hide resolved
lib/posix/strsignal.c Outdated Show resolved Hide resolved
tests/posix/common/src/signal.c Show resolved Hide resolved
@ycsin ycsin force-pushed the pr/posix-signal-apis branch from aef42c0 to 0b11d7a Compare July 14, 2023 13:35
cfriedt
cfriedt previously approved these changes Jul 14, 2023
@ycsin
Copy link
Member Author

ycsin commented Jul 17, 2023

@keith-packard may I have your review again?

#define SIGSYS 31 /**< Bad system call */

#define SIGRTMIN 32
#define SIGRTMAX (SIGRTMIN + CONFIG_POSIX_RTSIG_MAX)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you should define RTSIG_MAX to be CONFIG_POSIX_RTSIG_MAX in <limits.h> and that it must be at least 8.

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this Kconfig will make the implementation of limits.h a bit easier, but I think limits.h can probably be in another PR

Copy link
Member

@cfriedt cfriedt Jul 18, 2023

Choose a reason for hiding this comment

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

Yeah, that sounds like it's getting close to sysconf(). There is another PR in draft for that, so I can add RTSIG_MAX separately.

int "Maximum real-time signal number"
default 31
help
Define the maximum real-time signal number.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit confusing (as is the POSIX definition of the related RTSIG_MAX value). Might be good to clarify that this defines the POSIX value and that the range of RT signals is [SIGRTMIN .. (SIGRTMIN+RTSIG_MAX)]?

Copy link
Member Author

@ycsin ycsin Jul 18, 2023

Choose a reason for hiding this comment

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

Updated the desc to align with the doc:

config POSIX_RTSIG_MAX
	int "Maximum number of realtime signals"
	default 31
	help
	  Define the maximum number of realtime signals (RTSIG_MAX).
	  The range of realtime signals is [SIGRTMIN .. (SIGRTMIN+RTSIG_MAX)]

Initial header for the signal APIs.
APIs to be implemented in later commit.

Signed-off-by: Yong Cong Sin <[email protected]>
@ycsin ycsin force-pushed the pr/posix-signal-apis branch from 0b11d7a to c4c904e Compare July 18, 2023 02:34
@ycsin ycsin requested review from cfriedt and keith-packard July 18, 2023 02:36
@ycsin ycsin force-pushed the pr/posix-signal-apis branch from c4c904e to 522817d Compare July 18, 2023 03:04
#define SIGRTMAX (SIGRTMIN + CONFIG_POSIX_RTSIG_MAX)
#define _NSIG (SIGRTMAX + 1)

BUILD_ASSERT(CONFIG_POSIX_RTSIG_MAX >= 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

POSIX says 8; that could get checked here or Kconfig, or just stick a note in Kconfig indicating what will make the system POSIX compliant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added Kconfig.limits to specify the default limit to be posix compliant, but user can freely changes this Kconfig to be anything >= 0, let me know what you think about this approach

These Kconfigs serve as direct translation when we implement limits.h in the future

cc @cfriedt

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds perfect -- tell people how to make POSIX compliant systems, but also allow them to reduce resources if desired.

keith-packard
keith-packard previously approved these changes Jul 18, 2023
Copy link
Collaborator

@keith-packard keith-packard left a comment

Choose a reason for hiding this comment

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

Looking good.

ycsin added 8 commits July 18, 2023 13:07
Implementation and ztest for sigemptyset.

Signed-off-by: Yong Cong Sin <[email protected]>
Implementation and ztest for sigfillset.

Signed-off-by: Yong Cong Sin <[email protected]>
Implementation and ztest for sigaddset.

Signed-off-by: Yong Cong Sin <[email protected]>
Implementation and ztest for sigdelset.

Signed-off-by: Yong Cong Sin <[email protected]>
Implementation and ztest for sigismember.

Signed-off-by: Yong Cong Sin <[email protected]>
Implementation and ztest for strsignal.

Signed-off-by: Yong Cong Sin <[email protected]>
Add an extra test config to test large number of signals.

Signed-off-by: Yong Cong Sin <[email protected]>
Use build assert to make sure that the realtime signal
constants are configured properly in the Kconfig.

Signed-off-by: Yong Cong Sin <[email protected]>
@ycsin ycsin force-pushed the pr/posix-signal-apis branch from 8b5d7a8 to e1896cf Compare July 18, 2023 05:09
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

LGTM

@cfriedt cfriedt merged commit 43c5493 into zephyrproject-rtos:main Jul 18, 2023
@ycsin ycsin deleted the pr/posix-signal-apis branch July 19, 2023 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: CMSIS API Layer CMSIS-RTOS API Layer area: Coding Guidelines Coding guidelines and style area: Portability Standard compliant code, toolchain abstraction area: POSIX POSIX API Library
Projects
None yet
4 participants