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

sys/fs/constfs: drop dummy implementations #17651

Merged
merged 2 commits into from
May 19, 2022
Merged

sys/fs/constfs: drop dummy implementations #17651

merged 2 commits into from
May 19, 2022

Conversation

benpicco
Copy link
Contributor

Contribution description

VFS will handle it if those functions are not implemented.

Testing procedure

Issues/PRs references

@benpicco
sys/fs/constfs: drop dummy implementations
991f8e8 
VFS will handle it if those functions are not implemented.
@github-actions github-actions bot added the Area: sys Area: System label Feb 14, 2022
@benpicco benpicco added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Area: sys Area: System labels Feb 14, 2022
chrysn
chrysn reviewed Feb 14, 2022
View reviewed changes
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Generally a good change, I went through vfs.c to verify that it all still does the same (but did not do any testing yet).

sys/fs/constfs/constfs.c
(void) filp;
(void) src;
(void) nbytes;
return -EBADF;
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a change (vfs_write without a write returns -EINVAL), but I'd say it's for the better.

sys/fs/constfs/constfs.c
/* Removing files is prohibited */
(void) mountp; /* prevent warning: unused parameter */
(void) name; /* prevent warning: unused parameter */
return -EROFS;
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a change from -EPERM (which is the default) to -EROFS; probably OK (but may be worth a second thought).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't the default then rather be changed to -EROFS? The function not being implemented is not a permission issue.

Copy link
Member

Choose a reason for hiding this comment

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

I have no strong opinions on the concrete standard error values we return due to POSIXish conventions; EROFS sounds just as good as EPERM to me.

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 decided to change it to -EROFS in VFS.

@chrysn chrysn added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Feb 14, 2022
@github-actions github-actions bot added the Area: sys Area: System label May 18, 2022
chrysn
chrysn approved these changes May 19, 2022
View reviewed changes
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Looks good, did some light testing (on rm, not full coverage, but I think it's enough).

Thanks, and ACK.

@benpicco benpicco force-pushed the fs/constfs-cleanup branch from a235650 to a587069 Compare May 19, 2022 09:43
@benpicco benpicco requested a review from miri64 as a code owner May 19, 2022 09:43
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label May 19, 2022
@maribu maribu merged commit 158203f into RIOT-OS:master May 19, 2022
@benpicco benpicco deleted the fs/constfs-cleanup branch May 19, 2022 21:35
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrysn chrysn chrysn approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

Assignees
No one assigned
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
3 participants
@benpicco @chrysn @maribu