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 other _POSIX_THREAD_SAFE_FUNCTIONS #74350

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

posix: implement other _POSIX_THREAD_SAFE_FUNCTIONS #74350

wants to merge 3 commits into from
+396 −12

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented Jun 15, 2024 •
edited
Loading

Note

Has a commit from #74629

Time-related functions are in #74180.

This PR implements:

  • flockfile()
  • ftrylockfile()
  • funlockfile()
  • putc_unlocked()
  • putchar_unlocked()
  • getc_unlocked() (stub)
  • getchar_unlocked() (stub)

@ycsin ycsin requested a review from cfriedt June 15, 2024 16:47
@ycsin ycsin force-pushed the pr/posix_thread_safe_functions_1 branch 2 times, most recently from fd80070 to 740c14b Compare June 17, 2024 09:12
cfriedt
cfriedt requested changes Jun 17, 2024
View reviewed changes
lib/os/fdtable.c Outdated Show resolved Hide resolved
@cfriedt cfriedt added TSC Topics that need TSC discussion and removed TSC Topics that need TSC discussion labels Jun 17, 2024
@ycsin ycsin force-pushed the pr/posix_thread_safe_functions_1 branch from 740c14b to f69126e Compare June 18, 2024 08:04
This was linked to issues Jun 18, 2024
posix: implement flockfile() #74461
Open
posix: implement funlockfile() #74462
Open
posix: implement ftrylockfile() #74463
Open
posix: implement putc_unlocked() #74464
Open
posix: implement putchar_unlocked() #74465
Open
posix: implement getc_unlocked() #74466
Open
posix: implement getchar_unlocked() #74467
Open
@ycsin ycsin force-pushed the pr/posix_thread_safe_functions_1 branch from f69126e to a88e0ba Compare June 18, 2024 09:53
ycsin
ycsin commented Jun 18, 2024
View reviewed changes
tests/posix/common/src/file_locking.c Outdated
Comment on lines 10 to 19
#ifdef CONFIG_PICOLIBC
/* Not sure how to do this for picolibc */
void flockfile(FILE *file);
int ftrylockfile(FILE *file);
void funlockfile(FILE *file);
int putc_unlocked(int c, FILE *file);
int putchar_unlocked(int c);
int getc_unlocked(FILE *stream);
int getchar_unlocked(void);
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

@keith-packard do you have any idea on how to approach this?

The stdio.h that's being included here seems to be https://github.com/zephyrproject-rtos/picolibc/blob/main/newlib/libc/tinystdio/stdio.h, but https://github.com/zephyrproject-rtos/picolibc/blob/main/newlib/libc/stdio/stdio.h has the declaration I need.

Copy link
Member

Choose a reason for hiding this comment

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

If the declarations are missing from picolibc for whatever reason, I think this should be ok

Copy link
Member Author

@ycsin ycsin Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Removed them from picolibc as there is no way for any application to use them

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like we just need to add these to picolibc. which standard are these from?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in the _POSIX_THREAD_SAFE_FUNCTIONS option group

https://pubs.opengroup.org/onlinepubs/9699919799/functions/flockfile.html

Copy link
Collaborator

Choose a reason for hiding this comment

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

picolibc/picolibc#752

@cfriedt cfriedt self-requested a review June 20, 2024 12:29
cfriedt
cfriedt previously approved these changes Jun 20, 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.

I think this is fine too for the time being. Thanks for adding tests

@ycsin ycsin force-pushed the pr/posix_thread_safe_functions_1 branch from a88e0ba to aa3d548 Compare June 20, 2024 14:22
@ycsin ycsin marked this pull request as ready for review June 20, 2024 14:22
@zephyrbot zephyrbot added area: POSIX POSIX API Library area: Base OS Base OS Library (lib/os) area: C Library C Standard Library labels Jun 20, 2024
@ycsin ycsin force-pushed the pr/posix_thread_safe_functions_1 branch from aa3d548 to 59d5f21 Compare June 20, 2024 16:25
@ycsin ycsin marked this pull request as draft June 21, 2024 06:45
@ycsin ycsin force-pushed the pr/posix_thread_safe_functions_1 branch 3 times, most recently from 9679aaa to e8681e8 Compare June 24, 2024 05:56
@ycsin
posix: implement file locking functions
87729ba 
Implement file locking functions and added testcase.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
libc: minimal: implement putc_unlocked & putchar_unlocked
778629e 
Add a simple implementation for putc_unlocked() &
putchar_unlocked().

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin
libc: minimal: stubs getc_unlocked & getchar_unlocked
a206ee6 
Add stub implementation and test for `getc_unlocked()` &
`getchar_unlocked()`.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@ycsin ycsin force-pushed the pr/posix_thread_safe_functions_1 branch from e8681e8 to a206ee6 Compare June 24, 2024 06:54
cfriedt
cfriedt reviewed Jun 24, 2024
View reviewed changes
lib/libc/common/source/stdio/getc_unlocked.c
Comment on lines +12 to +19
int getc_unlocked(FILE *stream)
{
ARG_UNUSED(stream);

errno = ENOSYS;

return EOF;
}
Copy link
Member

Choose a reason for hiding this comment

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

For this one, I would add a zvfs_getc_unlocked() and just call that.

zvfs_getc_unlocked() should do some basic error checking but should not incur lock overhead.

E.g. check to see if stream is a valid pointer with IS_ARRAY_ELEMENT(), check the read() vtable method is non-NULL, etc.

@cfriedt cfriedt mentioned this pull request Jun 27, 2024
Open
@github-actions GitHub Actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 26, 2024
@github-actions github-actions bot closed this Sep 9, 2024
@ycsin ycsin removed the Stale label Sep 9, 2024
@ycsin ycsin reopened this Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfriedt cfriedt cfriedt left review comments

@keith-packard keith-packard Awaiting requested review from keith-packard

@jukkar jukkar Awaiting requested review from jukkar

@aescolar aescolar Awaiting requested review from aescolar

@andyross andyross Awaiting requested review from andyross

@dcpleung dcpleung Awaiting requested review from dcpleung

@nashif nashif Awaiting requested review from nashif

@peter-mitsis peter-mitsis Awaiting requested review from peter-mitsis

@stephanosio stephanosio Awaiting requested review from stephanosio

At least 2 approving reviews are required to merge this pull request.

Assignees

@cfriedt cfriedt

Labels
area: Base OS Base OS Library (lib/os) area: C Library C Standard Library area: POSIX POSIX API Library
Projects
None yet
Milestone
No milestone
4 participants
@ycsin @keith-packard @cfriedt @zephyrbot