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.

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

Jump to bottom

unixfs: fix dagTruncate to preserve node type #5216

Merged
merged 3 commits into from
Jul 18, 2018
Merged

unixfs: fix dagTruncate to preserve node type #5216

merged 3 commits into from
Jul 18, 2018

Conversation

schomatis
Copy link
Contributor

So, in a weird series of coincidences that I don't fully understand I need to fix this old bug (which I already fixed in the PR #4758 that I never finished) to be able to refactor some parts of the code of the DAG reader (see #5192 (comment)).

I'm selfishly assigning this PR a high priority to get it merged as soon as possible after the upcoming release, as it will simplify my work on the DAG reader.

Fixes #4519.

unixfs: fix `dagTruncate` to preserve node type

Extract the original `FSNode` passed inside the `ipld.Node` argument and modify
its `Blocksizes` (removing all of them and re-adding the ones that were not
truncated). In contrast, the replaced code was creating a new `FSNode` that was
not preserving some of the features of the original one.

mfs: add test case for MFS repeated truncation failure

@schomatis schomatis added kind/bug A bug in existing code (including security flaws) need/review Needs a review P1 High: Likely tackled by core team if no one steps up topic/MFS Topic MFS topic/UnixFS Topic UnixFS labels Jul 11, 2018
@schomatis schomatis self-assigned this Jul 11, 2018
@schomatis schomatis requested a review from Stebalien July 11, 2018 16:00
@schomatis schomatis requested a review from Kubuxu as a code owner July 11, 2018 16:00
@ghost ghost added the status/in-progress In progress label Jul 11, 2018
@schomatis schomatis added this to the Files API Documentation milestone Jul 11, 2018
@schomatis
Copy link
Contributor Author

schomatis commented Jul 11, 2018 •
edited
Loading

All of the truncation related tests of the shareness are failing as the TRUNC_HASH was probably built with this bug in place, in a similar situation I was explained that previous test hashes can't be changed (#5120 (comment)), so I'm not sure how to proceed here.

@schomatis
Copy link
Contributor Author

@whyrusleeping WDYT? (This is my most blocking issue at the moment.)

whyrusleeping
whyrusleeping approved these changes Jul 18, 2018
View reviewed changes
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This looks right to me. Good catch!

@whyrusleeping
Copy link
Member

On the TRUNC_HASH, I think its fine to change the hash here as this is clearly an erroneous object. We should probably also test that writing to a file, then truncating it back to the previous length results in the same object (unless there are reasons why this shouldnt happen)

@schomatis
Copy link
Contributor Author

I think its fine to change the hash here as this is clearly an erroneous object.

Great, do you know how was TRUNC_HASH generated?

We should probably also test that writing to a file, then truncating it back to the previous length results in the same object

Yes, will add that.

@schomatis schomatis removed need help need/review Needs a review labels Jul 18, 2018
@schomatis schomatis mentioned this pull request Jul 18, 2018
Merged
@schomatis
unixfs: fix dagTruncate to preserve node type
65a18ed 
Extract the original `FSNode` passed inside the `ipld.Node` argument and modify
its `Blocksizes` (removing all of them and re-adding the ones that were not
truncated). In contrast, the replaced code was creating a new `FSNode` that was
not preserving some of the features of the original one.

Change `TRUNC_HASH` values in `sharness` that were created with the bug to the
correct values.

License: MIT
Signed-off-by: Lucas Molas <[email protected]>
@schomatis
mfs: add test case for MFS repeated truncation failure
508af1e 
License: MIT
Signed-off-by: Lucas Molas <[email protected]>
@schomatis
unixfs/mod: add test to Truncate to the same size
70d0f13 
License: MIT
Signed-off-by: Lucas Molas <[email protected]>
@schomatis
Copy link
Contributor Author

Fixed TRUNC_HASH values and rebased to the merged PR #5253 fix so TestDagTruncateSameSize won't panic now.

@schomatis
Copy link
Contributor Author

Tests are passing now (minus Jenkins..) so @whyrusleeping I think this is RFM now.

@schomatis schomatis removed the status/in-progress In progress label Jul 18, 2018
@Stebalien Stebalien added the RFM label Jul 18, 2018
@whyrusleeping whyrusleeping merged commit e8cc529 into ipfs:master Jul 18, 2018
@schomatis schomatis deleted the fix/dag-modifier/preserve-fsnode branch July 18, 2018 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyrusleeping whyrusleeping whyrusleeping approved these changes

@Stebalien Stebalien Awaiting requested review from Stebalien

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@schomatis schomatis

Labels
kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up RFM topic/MFS Topic MFS topic/UnixFS Topic UnixFS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Truncating an MFS file 175 times makes it unwritable.
3 participants
@schomatis @whyrusleeping @Stebalien