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 more test file creation #256

Closed
wants to merge 1 commit into from
Closed

Conversation

mrbojangles3
Copy link

@mrbojangles3 mrbojangles3 commented Sep 18, 2024

I am using this project (its great!) I am seeing an issue when I am creating a bootable disk with two partitions. the first partition is a "normal" efi partition that boots and works well. The second partition is a fat32 filesystem that hold all sorts of files and folders. some of the files are 1 GiB in size. When I am writing to my second partition I see a segfault at the 3rd 1 GiB file. The others seem to do okay.

This is a PR that will create a tree on a partition and exposes the same error message that I was seeing on my system. The annoying thing is, I have to have the random-sized file creation turned on. to see my issue, so I don't have a simple test case. I saw that you called for a test program in #109 . This is hopefully that program. Happy to discuss further. Again, thank you for this project.

@mrbojangles3
Copy link
Author

I realized a little bit later that I can seed the random number generator so there can be a deterministic test case. I will update the code.

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

Hi @mrbojangles3

So when writing a 1GB file you get a segfault, but only on the 3rd one? Does this occur with any fat32 filesystem? Or only in a disk with 2 partitions?

If your test case can highlight the issue, then hopefully we can resolve it. Post here when you have the test complete, we can run it and then get it fixed.

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

Something weird about the go proxy. Leave it for an hour and we can rerun tests.

@mrbojangles3
Copy link
Author

@deitch , Thank you for your quick replies and willingness to work with on this.
I have updated the test code. The error I am seeing is:

$ ./fat32 
panic: could not read directory entries for /b/sub50/blob

goroutine 1 [running]:
main.mkGigFile({0x575550?, 0xc000120000?}, {0x5452a3?, 0x54e64b?})
        /home/lblyth/repos/go-diskfs/filesystem/fat32/testdata/fat32.go:133 +0xa5
main.main()
        /home/lblyth/repos/go-diskfs/filesystem/fat32/testdata/fat32.go:31 +0x331

My limited (but expanding) knowledge of the FAT file system make me suspect that a directory entry is getting overwritten somehow. In the test code if you remove lines 30 and 31, so that only a single Gig file is made, the error changes from:

panic: could not read directory entries for /b/sub50/blob

to

panic: error reading directory /b/sub50/blob: path /b not found

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

@mrbojangles3 is that the error you were seeing before? In other words, does this test case now faithfully reproduce your issue?

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

Why are you getting lint errors on unchanged files? That is really strange.

Yeah, I just ran it locally, same version as in there v1.59.0. Maybe go version, it is sensitive to 1.23 vs 1.22? I will try locally.

@Frostman
Copy link
Contributor

@deitch yes, it should match the go version too

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

Yeah, that is it. Sigh. I will fix them.

@mrbojangles3
Copy link
Author

To the best of my understanding, this reproduces my test case. On my system I actually see a a segfault. The path related errors are the same. my "/b" directory is unable to be found, when I call open on a file that will be a 1G file.

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

Oh, that linting is a pain. They include gosec, which has this aggressive G115, which complains about all of the integer overflow conversions, which are necessary.

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

That was harder than I wanted, but if you rebase, you should get past lint stuff

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

Do you mind squashing the commits do and cleaning up the base, so it is rebased on master, a clean single commit, rather than the merge commit?

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

go proxy misbehaving again.

@mrbojangles3
Copy link
Author

apologies, I think I squashed one commit too many. Since I have your lint changes in.

@mrbojangles3 mrbojangles3 reopened this Sep 19, 2024
@mrbojangles3
Copy link
Author

Okay, I think this is what you wanted.

@deitch
Copy link
Collaborator

deitch commented Sep 19, 2024

Yeah that looks right. Let's let CI run, and hope the go proxy issues have been resolved, but that's not us.

@deitch
Copy link
Collaborator

deitch commented Sep 20, 2024

So now I don't get it. You added a test that should fail, and the tests all pass?

@mrbojangles3
Copy link
Author

Apologies for being unclear. I modified the test code in fat32/testdata/fat32.go to show how I and perhaps the commenter in #109 were seeing issues with file creation, there was a request for example code. Perhaps I should have added this as a gist?

@deitch
Copy link
Collaborator

deitch commented Sep 22, 2024

I am confused. #109 said that there is a failure when writing certain files, and I think you had said the same thing. Isn't the test you added to trigger that failure? Such that the tests will fail, and then we can fix it such that the tests will pass?

@mrbojangles3
Copy link
Author

My takeaway from #109 was that an error occurred in file writing at around 50 files. I was seeing something similar in my usage of go-diskfs.
I have not modified the automated tests, but instead the code that resides inside of the testdata directory. I am still getting acclimated to the golang ecosystem and modifying the code I did was easier / quicker than adding a test.
I want to be a good contributor so it seems like the right thing to do is for me to modify one of the _internal_test.go files? I don't know for sure which internal test file will trigger the issue I have seen. But I suspect the fat32_internal_test.go?

Does the code I submitted run without issue for you?

@deitch
Copy link
Collaborator

deitch commented Sep 22, 2024

Yeah, it does. Look at the CI right here. No problems. If you have an issue, it would be great to get a reproduction into the tests.

@mrbojangles3
Copy link
Author

Closing to add test case in future PR

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.

3 participants