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

Add map_dir_from_tar and map_file_from_tar #508

Merged
merged 12 commits into from
Feb 19, 2024

Conversation

JSCU-CNI
Copy link
Contributor

As discussed in #492 (comment).

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (3852c0c) 73.94% compared to head (228f775) 73.99%.

Files Patch % Lines
dissect/target/filesystem.py 90.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #508      +/-   ##
==========================================
+ Coverage   73.94%   73.99%   +0.04%     
==========================================
  Files         284      284              
  Lines       23480    23498      +18     
==========================================
+ Hits        17362    17387      +25     
+ Misses       6118     6111       -7     
Flag Coverage Δ
unittests 73.99% <90.47%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Schamper
Copy link
Member

Is there a specific reason why a simple mount wouldn't suffice? Seems a bit wasteful to walk over the entire tar and map it into the VirtualFilesystem.

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Feb 1, 2024

Is there a specific reason why a simple mount wouldn't suffice? Seems a bit wasteful to walk over the entire tar and map it into the VirtualFilesystem.

I've simplified the helpers in 64a46b2. It is indeed not necessary to mount every file individually when using map_dir_from_tar. It now uses self.mount. However for map_file_from_tar we need to set the final destination vfspath manually, which can differ from the actual path inside the tar. Do you have a simpler solution?

@Schamper
Copy link
Member

Schamper commented Feb 2, 2024

I've simplified the helpers in 64a46b2. It is indeed not necessary to mount every file individually when using map_dir_from_tar. It now uses self.mount. However for map_file_from_tar we need to set the final destination vfspath manually, which can differ from the actual path inside the tar. Do you have a simpler solution?

This is to make some things work correctly with the .path/.name attribute of the newly mapped file entry, right? I think you're running into a similar problem I was having recently. It stems from the fact that we let filesystem entries have their own path/filename, instead of that being dictated by the directory listing they're in (as is done in every Unix filesystem. Turns out they knew what they were doing).

We plan to make a change there to introduce "DirEntry" objects sometime in the future, but as this is a pretty big change it won't come any time soon.

I went and found where I was having that problem. I opted to solve it by changing the .name attribute of the filesystem entry object. I think you could apply the same logic here, since you're not going to use it anymore. That would also make the stat change in VirtualFile redundant I believe.

https://github.com/fox-it/dissect.target/pull/419/files#diff-3714772d832a86a85264bb7dccebd6dd8b360f76745e89999df3de85f1c6f928R96-R98

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Feb 2, 2024

Thanks for the pointer @Schamper. Implemented the simplified mapping in 50ce246.

@Schamper Schamper merged commit 27a2068 into fox-it:main Feb 19, 2024
18 checks passed
@JSCU-CNI JSCU-CNI deleted the improvement/add-vfs-from-tar-funcs branch February 19, 2024 17:13
Zawadidone pushed a commit to Zawadidone/dissect.target that referenced this pull request Apr 5, 2024
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