-
Notifications
You must be signed in to change notification settings - Fork 155
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
File a minor race condition in the backup realm file action #7341
Conversation
03e4256
to
d09e596
Compare
Pull Request Test Coverage Report for Build thomas.goyne_172Details
💛 - Coveralls |
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.
LGTM
@@ -193,6 +193,13 @@ void migrate_to_v7(std::shared_ptr<Realm> old_realm, std::shared_ptr<Realm> real | |||
} | |||
} | |||
|
|||
void migrate_to_v8(Realm&, Realm& realm) | |||
{ | |||
if (auto app_metadata_table = realm.read_group().get_table("class_AppMetadata")) { |
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.
Will we need to do anything to convert the fileActionMetadata
entries to the new format without the partition
and identity
fields? If so, maybe add a comment somewhere in this function.
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.
No, removing columns no longer present in the schema is done automatically. It just requires a schema version bump since it's a destructive change.
Checking if the destination exists before copying is a race condition as the file can be created in between the check and the copy. Instead we should attempt to copy without overwriting the target if it exists.
d09e596
to
e31fc3b
Compare
Checking if the destination of a copy exists before trying to copy is a TOCTOU race because the state of the filesystem can change between when we check and when we perform the copy. We should instead just open the target file and not perform the copy if it already existed.
clonefile()
is an APFS feature to perform copy-on-write clones of files. This is much faster, and makes it so that the copy and then delete original that we do doesn't consume extra disk space. It's not always supported, so we fall back to manual copying if it fails.Two of the file action fields weren't actually used for anything.