Skip to content

Implement chunk comparison and selective extraction for borg extract (#5638) #8632

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implement chunk comparison and selective extraction for borg extract (#5638) #8632

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57760ef
Implement chunk comparison and selective extraction
alighazi288 Jan 12, 2025
80862e5
Refactor `compare_and_extract_chunks`
alighazi288 Jan 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 43 additions & 13 deletions src/borg/archive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,29 +747,60 @@ def compare_and_extract_chunks(self, item, fs_path, *, st, pi=None):
# Second pass: Update file
with backup_io("open"):
fs_file = open(fs_path, "rb+")
with fs_file:
Copy link
Member

@ThomasWaldmann ThomasWaldmann Mar 1, 2025

Choose a reason for hiding this comment

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

you removed the usage of the fs_file's context manager (removed the "with"). that means that it does not close the fs_file anymore (as it did when leaving the with-block), but I guess it should close the file.

for fs_chunk, item_chunk in zip(fs_chunks, item.chunks):
if fs_chunk == item_chunk:
with backup_io("seek"):
fs_file.seek(item_chunk.size, 1)
fs_file.seek(item_chunk.size, os.SEEK_CUR)
size = item_chunk.size
else:
chunk_data = next(chunk_data_iter)

with backup_io("write"):
fs_file.write(chunk_data)
size = len(chunk_data)
if pi:
pi.show(increase=len(chunk_data), info=[remove_surrogates(item.path)])
pi.show(increase=size, info=[remove_surrogates(item.path)])

final_size = fs_file.tell()
with backup_io("truncate_and_attrs"):
fs_file.truncate(item.size)
pos = item_chunks_size = fs_file.tell()
fs_file.truncate(pos)
fs_file.flush()

if not self.noacls:
try:
# Clear ACLs by setting empty ACL entries
acl_set(fs_path, {"acl_entries": []}, self.numeric_ids, fd=fs_file.fileno())
Copy link
Member

@ThomasWaldmann ThomasWaldmann Mar 1, 2025

Choose a reason for hiding this comment

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

you didn't read the acl_set code, did you?

except OSError as e:
if e.errno != errno.ENOTSUP: # Ignore if ACLs not supported
raise
if not self.noflags:
try:
# Clear all BSD flags
set_flags(fs_path, 0, fd=fs_file.fileno())
except OSError:
pass
if not self.noxattrs:
try:
# Clear ALL xattrs
attrs = xattr.listxattr(fs_file.fileno(), follow_symlinks=False)
for attr in attrs:
try:
xattr.setxattr(fs_file.fileno(), attr, b"", follow_symlinks=False)
Copy link
Member

@ThomasWaldmann ThomasWaldmann Mar 1, 2025

Choose a reason for hiding this comment

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

that does not do what you intended. setting an xattr to an empty bytestring is not the same as removing an xattr.

Copy link
Member

@ThomasWaldmann ThomasWaldmann Mar 1, 2025

Choose a reason for hiding this comment

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

you are calling fs_file.fileno() way too often.

except OSError as e:
if e.errno != xattr.ENOATTR: # Ignore if already removed
raise
except OSError as e:
if e.errno != errno.ENOTSUP: # Ignore if xattrs not supported
raise
self.restore_attrs(fs_path, item, fd=fs_file.fileno())
Copy link
Member

@ThomasWaldmann ThomasWaldmann Mar 1, 2025

Choose a reason for hiding this comment

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

you could create a method clear_attrs close to the definition of restore_attrs and move that code you added above to there.


if "size" in item and item.size != final_size:
raise BackupError(f"Size inconsistency detected: size {item.size}, chunks size {final_size}")
if "size" in item:
item_size = item.size
if item_size != item_chunks_size:
raise BackupError(f"Size inconsistency detected: size {item.size}, chunks size {item_chunks_size}")

if "chunks_healthy" in item and not item.chunks_healthy:
has_damaged_chunks = "chunks_healthy" in item
if has_damaged_chunks:
raise BackupError("File has damaged (all-zero) chunks. Try running borg check --repair.")

return True
Expand Down Expand Up @@ -857,16 +888,17 @@ def same_item(item, st):
st = os.stat(path, follow_symlinks=False)
if continue_extraction and same_item(item, st):
return # done! we already have fully extracted this file in a previous run.
if stat.S_ISREG(st.st_mode) and not continue_extraction:
if self.compare_and_extract_chunks(item, path, st=st, pi=pi):
return
elif stat.S_ISDIR(st.st_mode):
os.rmdir(path)
st = None
else:
os.unlink(path)
st = None
except UnicodeEncodeError:
raise self.IncompatibleFilesystemEncodingError(path, sys.getfilesystemencoding()) from None
except OSError:
st = None
pass

def make_parent(path):
parent_dir = os.path.dirname(path)
Expand All @@ -880,8 +912,6 @@ def make_parent(path):
with self.extract_helper(item, path, hlm) as hardlink_set:
if hardlink_set:
return
if self.compare_and_extract_chunks(item, path, st=st, pi=pi):
return

with backup_io("open"):
fd = open(path, "wb")
Expand Down
Loading