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

Fix #5, Remove OS_OpenCreate file status check in SC_LoadDefaultTables #81

Merged

Conversation

thnkslprpt
Copy link
Contributor

@thnkslprpt thnkslprpt commented Mar 21, 2023

Checklist

Describe the contribution

Testing performed
GitHub CI actions (incl. Build + Run, Unit Tests etc.) all passing successfully.

Expected behavior changes
Simplifies function and eases future maintenance.

Contributor Info
Avi Weiss @thnkslprpt

@dzbaker dzbaker requested a review from jphickey May 25, 2023 18:27
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

I'm not convinced this check should even be in there at all - no matter which function is used, its a race condition, due to the difference between the time of check and time of use.

That is, CFE_TBL_Load() will open the file again, and it should handle the error if it is not able to open it. SC should just check for that possible result - it shouldn't do its own check to avoid that result, because any such attempt would be incomplete and a race condition.

As this change set stands right now, while OS_stat() is lighter weight, it only verifies that the file exists, it does not verify that the file is readable.

I think we should simplify this, and remove this extra check.

@thnkslprpt thnkslprpt force-pushed the fix-5-convert-os-opencreate-to-os-stat branch from fae737a to 895d3fb Compare October 2, 2023 03:36
@thnkslprpt thnkslprpt changed the title Fix #5, Use OS_stat instead of OS_OpenCreate to verify file existence Fix #5, Remove OS_OpenCreate file status check in SC_LoadDefaultTables Oct 2, 2023
@thnkslprpt
Copy link
Contributor Author

I'm not convinced this check should even be in there at all...

Great!
Updated.

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

I like this simplification, but we need to check if this affects any of the build verification tests.

I think we should merge it but we need to note this may need a corresponding test update to go with it.

@thnkslprpt thnkslprpt force-pushed the fix-5-convert-os-opencreate-to-os-stat branch 2 times, most recently from 95d63ac to 77b34c7 Compare January 14, 2024 09:17
@thnkslprpt thnkslprpt force-pushed the fix-5-convert-os-opencreate-to-os-stat branch from 77b34c7 to 00a7577 Compare January 14, 2024 09:21
@jphickey
Copy link
Contributor

Thanks for the update, I still think we should merge this one. Will bring it up at the next review.

@dzbaker dzbaker merged commit 29f7bad into nasa:main Jan 25, 2024
17 checks passed
@thnkslprpt thnkslprpt deleted the fix-5-convert-os-opencreate-to-os-stat branch January 25, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use OS_stat to verify file existence
3 participants