-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Expose default file create permissions through a new OsCfg.fpp config file #4613
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
base: devel
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| mode_t PosixFile::map_open_create_mode(const U32 create_mode) { | ||
| mode_t out_mode = 0; |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
| break; | ||
| } | ||
| int descriptor = ::open(filepath, mode_flags, USER_FLAGS); | ||
| int descriptor = ::open(filepath, mode_flags, map_open_create_mode(Os::FILE_DEFAULT_CREATE_MODE)); |
Check notice
Code scanning / CodeQL
Use of basic integral type Note
|
|
||
| // Some posix systems (e.g. Darwin) use the older S_IREAD and S_IWRITE flags | ||
| // while other systems (e.g. Linux) use the newer S_IRUSR and S_IWUSR flags. | ||
| #if defined(S_IREAD) |
Check notice
Code scanning / CodeQL
Conditional compilation Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary to support Linux and MacOS (ie. BSD mode_t constants)
|
Note: we should clearly mark the configuration as Posix-specific |
| module Os { | ||
|
|
||
| @ File open mode permission bits | ||
| @ Constant values are derived from standard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CCB recommends that we namespace this further to make it clear it is meant to apply only to Posix. Other implementations may borrow from it if they want (e.g. VxWorks seems to have the same API?) but it should be clear that this isn't a universal config.
So something like such:
module Os {
module Posix {
[ ....]
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that this should be further namespaced.
The intention is that the FILE_MODE_ bits can be used across different backends. I only implemented it in the Posix backend because that's the only one I'm familiar with. However, the constants could be referenced by other backends. The Posix::File::map_open_create_mode maps these FILE_MODE_ bits to mode_t bits for Posix, but a similar function could be defined for VxWorks or zephyr to map FILE_DEFAULT_CREATE_MODE to the specific permission bits for those platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we want to avoid is for users to believe that this option will unambiguously happen anywhere, when that's not the case.
VxWorks and Zephyr happen to have the option to have some compliance with Posix w.r.t. file management, so I'd say it's not wrong to namespace Os::Posix when Posix is precisely what Zephyr/VxWorks are trying to comply with? (I might be reaching at straws haha)
I have no strong opinion, let's discuss with the team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we agree that, as much as possible, OSALs should attempt to respect this config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we agree that, as much as possible, OSALs should attempt to respect this config
If that's the case then I think that namespacing it under Os::Posix would discourage other OSALs from using the interface, and make it look like this is only intended to be a Posix thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very true!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, but could using an HPP file instead of FPP solve some of these concerns (conditional compilation / includes or whatnot)? Also easier to just reuse the Posix constants rather than re-define them in FPP
| @ File open mode permission bits | ||
| @ Constant values are derived from standard | ||
| @ Unix values, but should not be assumed | ||
| @ to match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should not be assumed to match? I'm not sure I follow.
Change Description
Creates an OsCfg.fpp configuration file with a default file creation mode constant. Os/Posix/File is updated to use this constant when creating new files. This allows the F Prime developer to control permissions on files created with Os::File, at least globally for the deployment
Rationale
This is a fix that would have been useful for a previous F Prime project, where the default 600 permissions caused operational headaches when the files were created by root, but access by a normal user
Testing/Review Recommendations
A small bit of test code was added to the reference topology to create files with this interface. All defined FILE_MODE_ bits were tested in one form or another on a Debian 13 system. The code was also compiled under Darwin and files were created with the expected permissions.
No testing was done on other operating systems. In particular, I am not sure how the VxWorks, FreeRTOS or Zephr OS layers will behave if they use the Os/Posix abstraction for File.
Future Work
I would also like to change the default creation mode from 600 to 666. That is the default used by multiple unix tools (touch, vim). End users can drop permissions by changing the config constant or by using umask with their application.
AI Usage (see policy)
No AI assistance used.