extract_item: do not delete an existing directory if possible#7866
extract_item: do not delete an existing directory if possible#7866intelfx wants to merge 1 commit intoborgbackup:masterfrom
Conversation
A pre-existing directory might be a Btrfs subvolume that was created by the user ahead of time when restoring several nested subvolumes from a single archive.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7866 +/- ##
==========================================
- Coverage 83.67% 83.65% -0.02%
==========================================
Files 66 66
Lines 11853 11854 +1
Branches 2149 2150 +1
==========================================
- Hits 9918 9917 -1
- Misses 1361 1362 +1
- Partials 574 575 +1 ☔ View full report in Codecov by Sentry. |
| elif not stat.S_ISDIR(item.mode): | ||
| os.rmdir(path) |
There was a problem hiding this comment.
It's rather unclear still whether the resulting directory after extraction will be correct (== as in the archive) in all cases:
- simple attrs
- xattrs
- acls
- (bsd/fs)flags
The existing directory could have some metadata set already, but the resulting directory must be exactly what we have in the archived item, not a mix up of fs item and archived item.
There was a problem hiding this comment.
The code that makes sure about this could be also useful later if we implement extracting into a non-empty directory (for more than the simplest cases, like the already implemented "continue an extraction").
There was a problem hiding this comment.
hmm, thinking about it...
we currently more or less require extracting into an empty dir (exception: that "continue extraction" feature). we could also require that if there is any pre-existing directory inside the extraction directory, it must not have any special attrs set.
| if not stat.S_ISDIR(st.st_mode): | ||
| os.unlink(path) | ||
| # only remove a directory if it is conflicting | ||
| # preserve existing directories because they might be subvolumes |
There was a problem hiding this comment.
is subvolumes a feature implemented by other fs than btrfs?
if not, maybe we better call it "btrfs subvolumes" here, so it is more clear.
|
A small test with an existing fs dir and then extracting "over" that would be good. |
|
@intelfx can you add tests? |
…up#4233 A pre-existing directory might be a btrfs subvolume that was created by the user ahead of time when restoring several nested subvolumes from a single archive. If the archive item to be extracted is a directory and there is already a directory at the destination path, do not remove (and recreate) it, but just use it. That way, btrfs subvolumes (which look like directories) are not deleted. Fix originally contributed by @intelfx in borgbackup#7866, but needed more work, so I thought more about the implications and added a test. Note: In the past, we first removed (empty) directories, then created a fresh one, then called restore_attrs for that. That produced correct metadata, but only for the case of an EMPTY exisiting directory. If the existing directory was not empty, the simply os.rmdir we tried did not work anyway and did not remove the existing directory. Usually we extract to an empty base directory, thus encountering this edge case is mostly limited to continuing a previous extraction. In that case, calling restore_attrs again on a directory that already has existing attrs should be harmless, because they are identical.
|
superceded by #9288. |
…up#4233 A pre-existing directory might be a btrfs subvolume that was created by the user ahead of time when restoring several nested subvolumes from a single archive. If the archive item to be extracted is a directory and there is already a directory at the destination path, do not remove (and recreate) it, but just use it. That way, btrfs subvolumes (which look like directories) are not deleted. Fix originally contributed by @intelfx in borgbackup#7866, but needed more work, so I thought more about the implications and added a test. Note: In the past, we first removed (empty) directories, then created a fresh one, then called restore_attrs for that. That produced correct metadata, but only for the case of an EMPTY exisiting directory. If the existing directory was not empty, the simply os.rmdir we tried did not work anyway and did not remove the existing directory. Usually we extract to an empty base directory, thus encountering this edge case is mostly limited to continuing a previous extraction. In that case, calling restore_attrs again on a directory that already has existing attrs should be harmless, because they are identical.
3 participants
A pre-existing directory might be a Btrfs subvolume that was created by the user ahead of time when restoring several nested subvolumes from a single archive.
I'm also interested in this feature being backported to Borg 1.2. I have a patch that applies on top of
1.2-maint.See: #4233