Skip to content

Truncate unnecessary data before specified offset #18

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

Merged
merged 5 commits into from
Oct 17, 2019
Merged

Truncate unnecessary data before specified offset #18

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
69b6671
Truncate unnecessary data before specified offset
ktock Sep 13, 2019
15a2736
Add more check to safely truncate unnecessary data.
ktock Sep 14, 2019
3d7e162
Add tests for offset and size calculation.
ktock Sep 29, 2019
fa18c71
Do not export unnecessary testing functions.
ktock Oct 15, 2019
cde2b63
Make the implementation simpler and easier to understand
ktock Oct 17, 2019
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
4 changes: 4 additions & 0 deletions crfs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,6 +1060,10 @@ func (h *nodeHandle) Read(ctx context.Context, req *fuse.ReadRequest, resp *fuse
if err != nil {
return err
}
if nr == 0 {
// Truncate unncessary data
chunkData = chunkData[req.Offset:]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right in general.

This is in a loop (line 1050) is will keep filling, but req.Offset should only be used for the first chunk. And even then, only the part between ce.ChunkOffset and req.Offset should probably be used.

If they wanted overall req.Offset 100 but the 2nd chunk of the file is at offset 90, then what you really want here is offset 10. You don't just want 100 each time.

This could probably use some tests, like the ones in the stargz package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. Right. I fixed offset calculation and added some checks in 15a2736.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this all just be:

    n := copy(resp.Data[nr:], chunkData[offset+int64(nr)-ce.ChunkOffset:])

With that change, all your tests still pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your idea. I think it's better too. I fixed it.

n := copy(resp.Data[nr:], chunkData)
nr += n
}
Expand Down