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

Set O_EXCL when opening file with include_excl #666

Merged
merged 1 commit into from
Jun 2, 2021

Conversation

Joshua-Anderson
Copy link
Contributor

@Joshua-Anderson Joshua-Anderson commented Jun 2, 2021

Originating Project/Creator
Affected Component Os
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n) y
Builds Without Errors (y/n) y
Unit Tests Pass (y/n) y
Documentation Included (y/n)

Change Description

When include_excl is set, set the O_EXCL flag. Previously this argument was unused (caught through gcc's unused parameter warning).

⚠️ Warning: This is a breaking change that silently changes default behavior.

Rationale

If we have an include_excl parameter, it should be respected. However, this does change the default behavior of File::open(const char* fileName, File::Mode mode), since it defaults include_excl to true. Because include_excl was ignored this effectively is equivalent to include_excl = false.

I think the default of not overwriting a file if it already exists is wise. Personally, I believe the value of enforcing this safer default is greater than the impact of breaking projects upgrading to F' 2.0 that implicitly depending on the overwrite on create behavior. However, this is a serious breaking change and it may make sense to keep the current unintentional default and update File::open(const char* fileName, File::Mode mode) to set the include_excl flag to false.

@Joshua-Anderson Joshua-Anderson requested a review from LeStarch June 2, 2021 00:35
@LeStarch
Copy link
Collaborator

LeStarch commented Jun 2, 2021

This should be discussed before we approve this change since it changes the behavior of an interface.

@LeStarch
Copy link
Collaborator

LeStarch commented Jun 2, 2021

:shipit:

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Since it only affects CREATE and that usage was corrected, I agree with the safer default. We'll add to upgrade notes to 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants