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

options/posix: sys_dup should not accept a flags parameter #534

Open
64 opened this issue Jun 23, 2022 · 8 comments
Open

options/posix: sys_dup should not accept a flags parameter #534

64 opened this issue Jun 23, 2022 · 8 comments
Labels
enhancement trivial issue Requires only straightforward changes and not new logic.

Comments

@64
Copy link
Member

64 commented Jun 23, 2022

Instead, there should be a sys_dup3 syscall as per Linux.

Alternatively we could keep it how it is and inside the Linux sysdeps use a fnctl(F_DUPFD_CLOEXEC) call to handle it.

@64 64 added enhancement trivial issue Requires only straightforward changes and not new logic. labels Jun 23, 2022
@avdgrinten
Copy link
Member

Hm? What prevents you from implementing sys_dup based on syscall(SYS_dup3) on Linux?

@64
Copy link
Member Author

64 commented Jun 23, 2022

What should the value of newfd be?

@avdgrinten
Copy link
Member

avdgrinten commented Jun 24, 2022

That's indeed an issue. I guess we should simply use fcntl(F_DUPFD_CLOEXEC) instead.

@ArsenArsen
Copy link
Member

That alters flags to add CLOEXEC. Matt discovered that the only use of the flags argument in sys_dup is in the managarm sysdep (the rest calls dup with zero), so we could just add an overload in that sysdep instead of adding the argument to the sysdep API

@avdgrinten
Copy link
Member

Well, there is only a single flag that can be set (O_CLOEXEC), so one can always either use F_DUPFD/dup (which unsets the flag) or F_DUPFD_CLOEXEC.

@ArsenArsen
Copy link
Member

The limit of which flags happen here could change in the future, though. It's more forward compatible to just refactor the Managarm sysdep

@64
Copy link
Member Author

64 commented Jun 24, 2022

I would also prefer to get rid of that parameter, but changing the other sysdeps might be a bit messy.

@ArsenArsen
Copy link
Member

why? AFAIK, this kind of change is why they're in tree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement trivial issue Requires only straightforward changes and not new logic.
Projects
None yet
Development

No branches or pull requests

3 participants