Skip to content

posix: fd_mgmt: implement dup(), dup2(), fseeko(), and ftello() #74096

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
Jun 27, 2024
Merged

posix: fd_mgmt: implement dup(), dup2(), fseeko(), and ftello() #74096

merged 2 commits into from
Jun 27, 2024

Conversation

@cfriedt
Copy link
Member

@cfriedt cfriedt commented Jun 11, 2024 •
edited
Loading

Implement the remaining functions from the POSIX_FD_MGMT Option Group that are part of POSIX, and add the CONFIG_REQUIRES_FULL_LIBC dependency to CONFIG_POSIX_FD_MGMT, to pull in the remaining C89 functions.

The POSIX_FD_MGMT Option Group is required for PSE52, PSE53, and PSE54 Subprofiles.

Note

If this goes in after #73978, the last commit in this PR can be dropped.

Doc Preview

Fixes #74097
Fixes #74098
Fixes #74099
Fixes #74100

@cfriedt cfriedt requested a review from ycsin June 11, 2024 14:37
@cfriedt cfriedt mentioned this pull request Jun 11, 2024
Open
4 tasks
@cfriedt cfriedt force-pushed the posix-fd-mgmt-fixups branch from f29decd to b541f7f Compare June 11, 2024 14:40
@cfriedt cfriedt changed the title posix: fd_mgmt: implement dup(), dup2(), and fseeko() posix: fd_mgmt: implement dup(), dup2(), fseeko(), and ftello() Jun 11, 2024
@cfriedt cfriedt force-pushed the posix-fd-mgmt-fixups branch 7 times, most recently from cd2853b to 00c8cf8 Compare June 15, 2024 19:38
@cfriedt cfriedt marked this pull request as ready for review June 17, 2024 15:50
@cfriedt cfriedt added the TSC Topics that need TSC discussion label Jun 17, 2024
@zephyrbot zephyrbot added area: POSIX POSIX API Library area: Base OS Base OS Library (lib/os) labels Jun 17, 2024
jukkar
jukkar previously approved these changes Jun 17, 2024
View reviewed changes
@cfriedt cfriedt removed the TSC Topics that need TSC discussion label Jun 17, 2024
@cfriedt cfriedt mentioned this pull request Jun 17, 2024
Closed
@vaishnavachath vaishnavachath self-requested a review June 17, 2024 20:02
vaishnavachath
vaishnavachath previously approved these changes Jun 17, 2024
View reviewed changes
ycsin
ycsin previously approved these changes Jun 18, 2024
View reviewed changes
lib/posix/options/fd_mgmt.c Outdated
Comment on lines 24 to 30
Copy link
Member

Choose a reason for hiding this comment

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

highlighting this just in case

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it is the last commit, and should be dropped prior to being merged

This was added so I didn't need to include the whole posix-device-io pr here too

ceolin
ceolin reviewed Jun 18, 2024
View reviewed changes
lib/os/fdtable.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be :

k_mutex_init(&fdtable[rt].lock);
k_condvar_init(&fdtable[ret].cond);

otherwise you are changing the original descriptor afaiu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh jeez - yes! Good catch!!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

lib/os/fdtable.c Outdated
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the new descriptor shares the same offset ?

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 possible that after a dup operation, the new FD might share the same offset. Let me double-check this behaviour on Linux and I'll update if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting - the POSIX spec does not specify whether they share the same offset after dup(), but in Linux, that's the case (and it's also stated in the Linux man page).

I think it's fine to follow the Linux convention here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@nashif nashif added this to the v4.0.0 milestone Jun 24, 2024
@cfriedt cfriedt added the DNM This PR should not be merged (Do Not Merge) label Jun 26, 2024
@cfriedt
Copy link
Member Author

cfriedt commented Jun 26, 2024

Temporary DNM until all comments have been addressed. Needs a rebase as well.

@nashif nashif modified the milestones: v4.0.0, v3.7.0 Jun 26, 2024
@cfriedt cfriedt dismissed stale reviews from ycsin, vaishnavachath, and jukkar via 1fd91c5 June 26, 2024 20:15
@cfriedt cfriedt force-pushed the posix-fd-mgmt-fixups branch 2 times, most recently from 1fd91c5 to 9772772 Compare June 26, 2024 20:15
@cfriedt
Copy link
Member Author

cfriedt commented Jun 26, 2024

  • rebased
  • ensured we initialize the ret mutex and cond instead of the fd mutex and cond
  • share offset from old fd in zvfs_dup()

Leaving DNM until posix-device-io PR is merged, and then I'll remove the last commit from this PR.

cfriedt added 2 commits June 27, 2024 11:41
@cfriedt
posix: fd_mgmt: implement dup(), dup2(), fseeko(), and ftello()
d92baf8 
Implement the remaining functions from the POSIX_FD_MGMT Option
Group that are part of POSIX, and add the
CONFIG_REQUIRES_FULL_LIBC dependency to CONFIG_POSIX_FD_MGMT, to
pull in the remaining C89 functions.

The POSIX_FD_MGMT Option Group is required for PSE52, PSE53, and
PSE54 Subprofiling Option Groups.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt
doc: posix: mark posis fd mgmt as supported
5e5dd21 
Mark the POSIX_FD_MGMT Option Group as supported.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt cfriedt force-pushed the posix-fd-mgmt-fixups branch from 9772772 to 5e5dd21 Compare June 27, 2024 15:42
@cfriedt cfriedt removed the DNM This PR should not be merged (Do Not Merge) label Jun 27, 2024
@cfriedt
Copy link
Member Author

cfriedt commented Jun 27, 2024

  • rebased

Please add a refresh +1 @ycsin @ceolin @jukkar @vaishnavachath

@nashif nashif merged commit e9b676a into zephyrproject-rtos:main Jun 27, 2024
@cfriedt cfriedt deleted the posix-fd-mgmt-fixups branch June 27, 2024 19:40
@aescolar aescolar mentioned this pull request Jul 1, 2024
Merged
@kartben kartben mentioned this pull request Jul 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nashif nashif nashif approved these changes

@ycsin ycsin ycsin approved these changes

@andyross andyross Awaiting requested review from andyross

@dcpleung dcpleung Awaiting requested review from dcpleung

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

@ceolin ceolin Awaiting requested review from ceolin

@jukkar jukkar Awaiting requested review from jukkar

@vaishnavachath vaishnavachath Awaiting requested review from vaishnavachath

Assignees

@cfriedt cfriedt

Labels

area: Base OS Base OS Library (lib/os) area: POSIX POSIX API Library

Projects

None yet

Milestone

v3.7.0

Development

Successfully merging this pull request may close these issues.

posix: fd_mgmt: implement ftello() posix: fd_mgmt: implement fseeko() posix: fd_mgmt: implement dup2() posix: fd_mgmt: implement dup()

7 participants

@cfriedt @nashif @ceolin @jukkar @ycsin @vaishnavachath @zephyrbot