Skip to content

doc: posix: mark putmsg as supported #68502

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 16, 2024
Merged

doc: posix: mark putmsg as supported #68502

merged 2 commits into from
Mar 16, 2024

Conversation

moonlight83340
Copy link
Contributor

putmsg() is missing in the documentation.
Has been discuss here #67711

@zephyrbot zephyrbot added Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. area: POSIX POSIX API Library labels Feb 3, 2024
@zephyrbot zephyrbot requested review from cfriedt and ycsin February 3, 2024 15:12
cfriedt
cfriedt previously approved these changes Feb 3, 2024
View reviewed changes
nashif
nashif previously requested changes Feb 3, 2024
View reviewed changes
doc/services/portability/posix/option_groups/index.rst Outdated
@@ -509,7 +509,7 @@ _XOPEN_STREAMS
getpmsg(),
ioctl(),yes
isastream(),
putmsg(),
putmsg(),yes
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct, putmsg was not fully implemented and we just provide an empty stub, can we clarify please, otherwise it is false advertisement, "yes" should be used only for fully implemented functions IMO, for empty stubs, lets use something else to make it clear.

Copy link
Member

@cfriedt cfriedt Feb 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes - good catch.

@moonlight83340 - can you please match what was done in the docs for other functions where ENOSYS is reported?

Also, in a second commit, it would be great of you could clean up any other ENOSYS functions that have similar entries in the docs.

Copy link
Contributor Author

@moonlight83340 moonlight83340 Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

in posix directory, I saw we still put the yes in the documentation even when there are ENOSYS 😓
Is there any other part of the documentation that should be modify ? after a quick search for ENOSYS functions and documentation files, I'm not seing others changes.

From here : #66975 (comment)
Maybe I should add something to precise that it is currently "undefined behaviour" ?

Copy link
Member

Choose a reason for hiding this comment

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

Please copy the .rst source where it says

"(will fail with ENOSYS†)"

https://docs.zephyrproject.org/latest/services/portability/posix/conformance/index.html#posix-system-interfaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I keep the yes with the "(will fail with ENOSYS†)" in the compliance documentation part ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not correct, putmsg was not fully implemented and we just provide an empty stub, can we clarify please, otherwise it is false advertisement, "yes" should be used only for fully implemented functions IMO, for empty stubs, lets use something else to make it clear.

Do you think modifications are okay ? 😅

cfriedt
cfriedt reviewed Feb 4, 2024
View reviewed changes
doc/services/portability/posix/option_groups/index.rst Outdated
@@ -509,7 +509,7 @@ _XOPEN_STREAMS
getpmsg(),
ioctl(),yes
isastream(),
putmsg(),
putmsg(),yes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
putmsg(),yes
putmsg(), yes (will fail with ``ENOSYS``:ref:`†<posix_undefined_behaviour>`)

cfriedt
cfriedt reviewed Feb 4, 2024
View reviewed changes
doc/services/portability/posix/conformance/index.rst Outdated
@@ -109,7 +109,7 @@ POSIX System Interfaces
_XOPEN_CRYPT, -1,
_XOPEN_REALTIME, -1,
_XOPEN_REALTIME_THREADS, -1,
:ref:`_XOPEN_STREAMS<posix_option_xopen_streams>`, -1, :kconfig:option:`CONFIG_NET_SOCKETS`
:ref:`_XOPEN_STREAMS<posix_option_xopen_streams>`, -1, :kconfig:option:`CONFIG_NET_SOCKETS` (will fail with ``ENOSYS``:ref:`†<posix_undefined_behaviour>`)
Copy link
Member

@cfriedt cfriedt Feb 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Once all prototypes for _XOPEN_STREAMS are available (at the interface level so that an application can link), this can be changed to 200809L instead of -1. But the reference to CONFIG_NET_SOCKETS is invalid, so please remove it.

Suggested change
:ref:`_XOPEN_STREAMS<posix_option_xopen_streams>`, -1, :kconfig:option:`CONFIG_NET_SOCKETS` (will fail with ``ENOSYS``:ref:`†<posix_undefined_behaviour>`)
:ref:`_XOPEN_STREAMS<posix_option_xopen_streams>`, -1, :ref:`†<posix_undefined_behaviour>`

@moonlight83340
Copy link
Contributor Author

moonlight83340 commented Feb 5, 2024 •
edited
Loading

Also in lib/posix/options/sched.c , all the function are marks yes but no mention of enosys. Should it be the same as here ?
I have done my self #68362 a modification on yes and now we discuss about it it seems it should have the mention enosys also.

Possibily have to modify the documentation of :
#68200
#68316
#67028

@cfriedt
Copy link
Member

cfriedt commented Feb 5, 2024 •
edited
Loading

Possibily have to modify the documentation of :
#68200
#68316

@moonlight83340 - thanks

I think the above two are all that are necessary, the 3rd PR looks like it does mention ENOSYS

@moonlight83340
Copy link
Contributor Author

Possibily have to modify the documentation of :
#68200
#68316

@moonlight83340 - thanks

I think the above two are all that are necessary, the 3rd PR looks like it does mention ENOSYS

Should I do it in a different PR ?

@moonlight83340 moonlight83340 mentioned this pull request Feb 6, 2024
Merged
@moonlight83340 moonlight83340 requested a review from nashif February 7, 2024 07:00
@moonlight83340
Copy link
Contributor Author

Could be good to pass the documentation precision before 3.6 is released, should I chnage the PR and add the others modifications or do anothers PR for the others documentation modification ?

@cfriedt
Copy link
Member

cfriedt commented Feb 16, 2024

Should I do it in a different PR ?

@moonlight83340 - one pr should be fine

@moonlight83340 moonlight83340 force-pushed the doc_putmsg branch 2 times, most recently from 2247895 to ad69500 Compare February 29, 2024 07:40
@moonlight83340
doc: posix: mark putmsg as supported
f626abc 
Mark `putmsg()` as implemented but will fail with ``ENOSYS``
Was missing on the documentation before.

signed-off-by: Gaetan Perrot <[email protected]>
@moonlight83340
doc: posix: sched functions will fail with ENOSYS
5ca8105 
Mark `sched_setparam()` , `sched_setscheduler()`
and `sched_rr_get_interval()`
have implemented but will fail with ``ENOSYS``
Were missing on the documentation before.

signed-off-by: Gaetan Perrot <[email protected]>
@moonlight83340
Copy link
Contributor Author

I should change the PR name for something else, I think..

@cfriedt cfriedt requested a review from kartben February 29, 2024 10:56
@cfriedt cfriedt dismissed nashif’s stale review March 16, 2024 09:17

I believe this was addressed

@cfriedt cfriedt merged commit b519041 into zephyrproject-rtos:main Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfriedt cfriedt cfriedt approved these changes

@ycsin ycsin ycsin approved these changes

@nashif nashif Awaiting requested review from nashif

@kartben kartben Awaiting requested review from kartben

Assignees

@cfriedt cfriedt

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@moonlight83340 @cfriedt @nashif @ycsin @zephyrbot