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

mfs: seek to 0 before reading in TestTruncateAndWrite #5262

Merged
merged 1 commit into from
Jul 23, 2018
Merged

mfs: seek to 0 before reading in TestTruncateAndWrite #5262

merged 1 commit into from
Jul 23, 2018

Conversation

schomatis
Copy link
Contributor

Right now this test isn't failing due to another bug that causes seeking to the end to return to the beginning of the file (#5255) and the read call works, but it would be nice to get this test fixed as it's failing in a refactoring of the DAG reader (#5257).

@schomatis schomatis added kind/bug A bug in existing code (including security flaws) need/review Needs a review topic/MFS Topic MFS labels Jul 19, 2018
@schomatis schomatis added this to the Files API Documentation milestone Jul 19, 2018
@schomatis schomatis self-assigned this Jul 19, 2018
@schomatis schomatis requested a review from Stebalien July 19, 2018 03:22
@schomatis schomatis requested a review from Kubuxu as a code owner July 19, 2018 03:22
@ghost ghost added the status/in-progress In progress label Jul 19, 2018
@schomatis schomatis removed the status/in-progress In progress label Jul 19, 2018
Stebalien
Stebalien approved these changes Jul 19, 2018
View reviewed changes
mfs/mfs_test.go Outdated
data, err := ioutil.ReadAll(fd)
if err != nil {
t.Fatal(err)
}
if string(data) != "test" {
t.Errorf("read error at read %d, read: %v", i, data)
t.Fatal(fmt.Errorf("read error at read %d, read: %v", i, data))
Copy link
Member

Choose a reason for hiding this comment

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

Can just use Fatalf.

@whyrusleeping
Copy link
Member

@schomatis so ftruncate in standard unix doesnt touch the current offset of the file descriptor, right?

@Stebalien
Copy link
Member

No but the write does. However, yeah, this should probably require a seek after the truncate as well. That looks like a different issue.

@schomatis
mfs: seek to 0 before reading in TestTruncateAndWrite
acf12b6 
License: MIT
Signed-off-by: Lucas Molas <schomatis@gmail.com>
@ghost ghost added the status/in-progress In progress label Jul 19, 2018
@schomatis
Copy link
Contributor Author

Can just use Fatalf.

Fixed, thanks for the tip!

@schomatis
Copy link
Contributor Author

schomatis commented Jul 20, 2018 •
edited
Loading

@schomatis so ftruncate in standard unix doesnt touch the current offset of the file descriptor, right?

Yes, actually the Truncate function isn't modifying the offset, but there seems to be another (different) bug in how writeStart is handled that is compensating for the lack of a seek after truncation.

The writeStart offset never syncs its position with curWrOff and keeps growing without limit and Sync() (incorrectly) interprets it as "at EOF, need to append", so the write happens (as an append to an empty file DAG, which is the same as a write at offset zero) and the test succeeds. I'll submit a separate issue for this.

EDIT: Actually the error is at modifyDag() where the offset was left at 4 but it's writing at the beginning of the file (appending a node to the end of an empty file DAG technically).

@schomatis
Copy link
Contributor Author

Some sort of republish error at CircleCI but the other tests are passing, RFM.

@schomatis schomatis added RFM and removed status/in-progress In progress need/review Needs a review labels Jul 20, 2018
@schomatis schomatis mentioned this pull request Jul 20, 2018
Closed
whyrusleeping added a commit that referenced this pull request Jul 23, 2018
@whyrusleeping
Merge pull request #5262 from schomatis/fix/mfs/test/seek-before-read
ec61e06 
mfs: seek to 0 before reading in `TestTruncateAndWrite`
@whyrusleeping whyrusleeping merged commit ec61e06 into ipfs:master Jul 23, 2018
@whyrusleeping whyrusleeping merged commit acf12b6 into ipfs:master Jul 23, 2018
@whyrusleeping
Copy link
Member

Thanks @schomatis!

@schomatis schomatis deleted the fix/mfs/test/seek-before-read branch July 23, 2018 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@schomatis schomatis

Labels
kind/bug A bug in existing code (including security flaws) RFM topic/MFS Topic MFS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@schomatis @whyrusleeping @Stebalien