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.

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

posix: implement isastream() #68458

Merged
merged 3 commits into from
Apr 14, 2024
Merged

posix: implement isastream() #68458

merged 3 commits into from
Apr 14, 2024

Conversation

moonlight83340
Copy link
Contributor

This is part of the See #51211 (RFC #51211).

isastream() is required as part of _XOPEN_STREAMS Option Group.

For more information, please refer to https://pubs.opengroup.org/onlinepubs/9699919799/functions/isastream.html

Fixes #66977

@moonlight83340 moonlight83340 marked this pull request as ready for review February 2, 2024 12:45
@zephyrbot zephyrbot added the area: POSIX POSIX API Library label Feb 2, 2024
@zephyrbot zephyrbot requested review from cfriedt and ycsin February 2, 2024 12:46
cfriedt
cfriedt previously approved these changes Feb 3, 2024
View reviewed changes
ycsin
ycsin previously approved these changes Feb 5, 2024
View reviewed changes
nashif
nashif reviewed Feb 5, 2024
View reviewed changes
doc/services/portability/posix/option_groups/index.rst
@@ -508,7 +508,7 @@ _XOPEN_STREAMS
getmsg(),
getpmsg(),
ioctl(),yes
isastream(),
Copy link
Member

Choose a reason for hiding this comment

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

that is not supported as in "implemented", so we need to change the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking on : yes (will fail with ENOSYS:ref:†<posix_undefined_behaviour>) like the other one ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thanks

nashif
nashif previously requested changes Feb 5, 2024
View reviewed changes
Copy link
Member

@nashif nashif left a comment

Choose a reason for hiding this comment

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

"supported" in the docs is misleading.

@moonlight83340 moonlight83340 dismissed stale reviews from ycsin and cfriedt via da4ffae February 6, 2024 02:47
@moonlight83340 moonlight83340 force-pushed the isastream_XOPEN_STREAMS branch from b6ece1a to da4ffae Compare February 6, 2024 02:47
@moonlight83340
Copy link
Contributor Author

moonlight83340 commented Feb 6, 2024 •
edited
Loading

"supported" in the docs is misleading.

I add in the documentation, that it will faill with ENOSYS, the function is present but it's a placeholder.
See #68502
I have also modify #68464

cfriedt
cfriedt previously approved these changes Feb 8, 2024
View reviewed changes
@moonlight83340 moonlight83340 force-pushed the isastream_XOPEN_STREAMS branch 2 times, most recently from c20aef1 to cadebfa Compare March 17, 2024 09:47
@moonlight83340
Copy link
Contributor Author

Not sure, on the problem, I check the upstream and keep my changes. I have check the code 🤔
Any idea @cfriedt ?

@cfriedt
Copy link
Member

cfriedt commented Mar 18, 2024 •
edited
Loading

Not sure, on the problem, I check the upstream and keep my changes. I have check the code 🤔
Any idea @cfriedt ?

Hi @moonlight83340 - If a test is failing, just click on the failing test, and click through to the failing job. You might need to search through the log messages to see what is failing. The failure is completely unrelated to this change, so your PR might require a rebase if the issue it's already fixed in main.

Screenshot_20240318-082124.png

Screenshot_20240318-082442.png

https://github.com/zephyrproject-rtos/zephyr/actions/runs/8314532525/job/22752673417#step:12:1704

In this case, it's an NXP hal issue that is causing a build failure, so is completely unrelated.

@moonlight83340
Copy link
Contributor Author

Not sure, on the problem, I check the upstream and keep my changes. I have check the code 🤔
Any idea @cfriedt ?

Hi @moonlight83340 - If a test is failing, just click on the failing test, and click through to the failing job. You might need to search through the log messages to see what is failing. The failure is completely unrelated to this change, so your PR might require a rebase if the issue it's already fixed in main.

Screenshot_20240318-082124.png

Screenshot_20240318-082442.png

https://github.com/zephyrproject-rtos/zephyr/actions/runs/8314532525/job/22752673417#step:12:1704

In this case, it's an NXP hal issue that is causing a build failure, so is completely unrelated.

Thank you, I ask because I check and the error sounded unrelated but I wasn't sure ! Thank you for your time !

@moonlight83340
posix: Implement isastream
ce240c1 
`isastream()` is required as part of _XOPEN_STREAMS Option Group.

signed-off-by: Gaetan Perrot <gaetanperrotpro@gmail.com>
@moonlight83340
doc: posix: mark isastream as supported
5e37757 
`isastream ()` is now implemented, mark it so.

signed-off-by: Gaetan Perrot <gaetanperrotpro@gmail.com>
@moonlight83340
posix: Implement test for isastream
d2469b2 
Add tests for `isastream()`

signed-off-by: Gaetan Perrot <gaetanperrotpro@gmail.com>
@moonlight83340 moonlight83340 force-pushed the isastream_XOPEN_STREAMS branch from cadebfa to d2469b2 Compare March 30, 2024 11:03
@moonlight83340 moonlight83340 requested a review from cfriedt March 31, 2024 09:36
cfriedt
cfriedt approved these changes Mar 31, 2024
View reviewed changes
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.

Thanks - LGTM 👍

@moonlight83340
Copy link
Contributor Author

@nashif are the changes okay ?

@cfriedt cfriedt merged commit 0b51d37 into zephyrproject-rtos:main Apr 14, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ycsin ycsin ycsin approved these changes

@cfriedt cfriedt cfriedt approved these changes

@nashif nashif Awaiting requested review from nashif

Assignees

@cfriedt cfriedt

Labels
area: POSIX POSIX API Library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

posix: implement isastream()
5 participants
@moonlight83340 @cfriedt @nashif @ycsin @zephyrbot